Close detached DevTools window (rather than open a new instance) when it receives an F12 keypress

Close detached DevTools window (rather than open a new instance) when it receives an F12 keypress


BUG=139581
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154694

[apavlov, 2012-08-08 15:41:24]

Folks, could you take a look at the patch?

This is take 2 of fixing bug 139581, but this time only F12 results in toggling
the DevTools window (i.e. closing it when it is focused). We got a few
complaints from our users for when they could not inspect an existing DevTools
instance.

[Nico, 2012-08-13 22:45:50]

(I'm waiting for pfeldman to look first. I suppose you're intentionally not
changing any mac files.)

[pfeldman, 2012-08-14 11:04:11]

http://codereview.chromium.org/10823233/diff/1/chrome/browser/debugger/devtoo...
File chrome/browser/debugger/devtools_toggle_action.h (right):

http://codereview.chromium.org/10823233/diff/1/chrome/browser/debugger/devtoo...
chrome/browser/debugger/devtools_toggle_action.h:12:
DEVTOOLS_TOGGLE_ACTION_TOGGLE_INSPECTOR
I'd rename _NONE to _SHOW and add _TOGGLE for better clarity.

http://codereview.chromium.org/10823233/diff/1/chrome/browser/debugger/devtoo...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10823233/diff/1/chrome/browser/debugger/devtoo...
chrome/browser/debugger/devtools_window.cc:613: if (action ==
DEVTOOLS_TOGGLE_ACTION_TOGGLE_INSPECTOR) {
You should add event source browser here. Then you would do
if (browser->is_devtools())
    chrome::CloseAllTabs(browser);

http://codereview.chromium.org/10823233/diff/1/chrome/browser/debugger/devtoo...
File chrome/browser/debugger/devtools_window.h (right):

http://codereview.chromium.org/10823233/diff/1/chrome/browser/debugger/devtoo...
chrome/browser/debugger/devtools_window.h:137: static DevToolsWindow*
AsDevToolsWindow(content::RenderViewHost*);
You don't need this.

[pfeldman, 2012-08-24 10:16:14]

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:106: return
AsDevToolsWindow(window_rvh) != NULL;
drop the != NULL

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:151: return
AsDevToolsWindow(inspected_rvh);
This is always NULL.

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_command_controller.cc (right):

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller.cc:958:
command_updater_.UpdateCommandEnabled(IDC_DEV_TOOLS_TOGGLE_INSPECTOR,
_TOGGLE ? (Drop INSPECTOR)

[apavlov, 2012-08-24 11:02:13]

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:106: return
AsDevToolsWindow(window_rvh) != NULL;
On 2012/08/24 10:16:14, pfeldman wrote:
> drop the != NULL

Done.

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:151: return
AsDevToolsWindow(inspected_rvh);
On 2012/08/24 10:16:14, pfeldman wrote:
> This is always NULL.

This is NULL for ordinary RVH's but not for the DevTools window RVH (when
browser->is_devtools()).

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/ui/browser_c...
File chrome/browser/ui/browser_command_controller.cc (right):

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/ui/browser_c...
chrome/browser/ui/browser_command_controller.cc:958:
command_updater_.UpdateCommandEnabled(IDC_DEV_TOOLS_TOGGLE_INSPECTOR,
On 2012/08/24 10:16:14, pfeldman wrote:
> _TOGGLE ? (Drop INSPECTOR)

Done.

[pfeldman, 2012-08-24 11:23:19]

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:151: return
AsDevToolsWindow(inspected_rvh);
- So browser _is_ the devtools window
- Then there is no point in getting its active web contents (this is the
devtools front-end page). It is not _inspected_, so the code above is wrong.
- And AsDevToolsWindow for it will return the devtools that inspects the
devtools. It all sounds completely wrong.

[apavlov, 2012-08-24 11:34:39]

On 2012/08/24 11:23:19, pfeldman wrote:
>
http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
> File chrome/browser/debugger/devtools_window.cc (right):
> 
>
http://codereview.chromium.org/10823233/diff/6001/chrome/browser/debugger/dev...
> chrome/browser/debugger/devtools_window.cc:151: return
> AsDevToolsWindow(inspected_rvh);
> - So browser _is_ the devtools window
> - Then there is no point in getting its active web contents (this is the
> devtools front-end page). It is not _inspected_, so the code above is wrong.
> - And AsDevToolsWindow for it will return the devtools that inspects the
> devtools. It all sounds completely wrong.

Yep, I wasn't sure what we should return as the window for the DevTools window
(now that we have this case). NULL seems a good choice for me :) Fixed.

[pfeldman, 2012-08-27 09:21:33]

http://codereview.chromium.org/10823233/diff/7002/chrome/browser/debugger/dev...
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/10823233/diff/7002/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:626: if (host && window) {
!window

http://codereview.chromium.org/10823233/diff/7002/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:669: DevToolsWindow*
DevToolsWindow::AsDevToolsWindow(RenderViewHost* rvh) {
window_rvh

http://codereview.chromium.org/10823233/diff/7002/chrome/browser/debugger/dev...
chrome/browser/debugger/devtools_window.cc:670: if (g_instances == NULL)
if (!g_instance)

[apavlov, 2012-08-30 14:58:40]

On 2012/08/27 09:21:33, pfeldman wrote:
http://codereview.chromium.org/10823233/diff/7002/chrome/browser/debugger/dev...
> chrome/browser/debugger/devtools_window.cc:670: if (g_instances == NULL)
> if (!g_instance)

This does not compile. Fixed the rest.

Yury, could you lgtm the patch please? I've addressed the comments.

[pfeldman, 2012-08-30 14:59:54]

lgtm

[Nico, 2012-08-30 15:32:49]

lgtm

[commit-bot: I haz the power, 2012-08-30 16:11:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/10823233/11012

[commit-bot: I haz the power, 2012-08-30 16:42:31]

Try job failure for 10823233-11012 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2012-08-31 16:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/10823233/22002

[commit-bot: I haz the power, 2012-08-31 17:04:05]

Try job failure for 10823233-22002 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-31 17:36:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/10823233/25003

[commit-bot: I haz the power, 2012-08-31 18:41:13]

Try job failure for 10823233-25003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-03 11:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/apavlov@chromium.org/10823233/20006

[commit-bot: I haz the power, 2012-09-03 14:25:12]

Change committed as 154694