Add "Inspect Element" shortcut to browser tabs

Add "Inspect Element" shortcut to browser tabs.

If the DevTools window is closed, it gets opened, and the Inspect Element tool is enabled. If the DevTools window is opened, the inspected tab remains focused with the Inspect Element tool enabled.
If the Inspect Element tool is enabled, it gets turned off.

The shortcut is Ctrl-Shift-C for Win/Linux, Cmd-Shift-C for Mac

BUG=29532
TEST=manual

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

[apavlov, 2010-07-15 12:41:14]

Please have a look.

[yurys, 2010-07-15 15:59:42]

http://codereview.chromium.org/3011002/diff/2001/3008
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/3011002/diff/2001/3008#newcode131
chrome/browser/debugger/devtools_window.cc:131: else if (action ==
DEVTOOLS_TOGGLE_ACTION_INSPECT)
I'd rather use schedule an action and have a switch only in DoAction method.
This way we wouldn't double flags and methods.

[sky, 2010-07-15 16:00:51]

Do you really need all of us to review the entire patch? When picking
multiple reviewers you should make it clear what you would like each
reviewer to look at.

  -Scott

On Thu, Jul 15, 2010 at 5:41 AM,  <apavlov@chromium.org> wrote:
> Reviewers: pfeldman, Yury Semikhatsky, ananta, mpcomplete, sky, Nico,
>
> Message:
> Please have a look.
>
> Description:
> Add "Inspect Element" shortcut to browser tabs.
>
> If the DevTools window is closed, it gets opened, and the Inspect Element
> tool
> is enabled. If the DevTools window is opened, the inspected tab remains
> focused
> with the Inspect Element tool enabled.
> If the Inspect Element tool is enabled, it gets turned off.
>
> The shortcut is Ctrl-Shift-C for Win/Linux, Cmd-Shift-C for Mac
>
> BUG=29532
> TEST=manual
>
> Please review this at http://codereview.chromium.org/3011002/show
>
> Affected files:
>  M chrome/app/chrome_dll.rc
>  M chrome/app/chrome_dll_resource.h
>  M chrome/browser/browser.h
>  M chrome/browser/browser.cc
>  M chrome/browser/debugger/devtools_manager.h
>  M chrome/browser/debugger/devtools_manager.cc
>  A chrome/browser/debugger/devtools_toggle_action.h
>  M chrome/browser/debugger/devtools_window.h
>  M chrome/browser/debugger/devtools_window.cc
>  M chrome/browser/extensions/extensions_ui.cc
>  M chrome/browser/external_tab_container_win.cc
>  M chrome/browser/global_keyboard_shortcuts_mac.mm
>  M chrome/browser/gtk/accelerators_gtk.cc
>  M chrome/browser/views/accelerator_table_gtk.cc
>  M chrome/browser/views/extensions/extension_popup.cc
>  M chrome/chrome.gyp
>
>
>

[apavlov, 2010-07-15 16:07:44]

On 2010/07/15 16:00:51, sky wrote:
> Do you really need all of us to review the entire patch? When picking
> multiple reviewers you should make it clear what you would like each
> reviewer to look at.

Apologies if this has been misunderstood. The real intent was in fact that
everyone looks only at the files they have touched (as this change involves
changes to platform-specific code for all platforms, as well as extension- and
Chrome Frame-related stuff, which is well beyond my competence, as well as that
of my teammates). No need to check the code you have never worked with.

[Nico, 2010-07-15 17:02:29]

http://codereview.chromium.org/3011002/diff/2001/3012
File chrome/browser/global_keyboard_shortcuts_mac.mm (right):

http://codereview.chromium.org/3011002/diff/2001/3012#newcode68
chrome/browser/global_keyboard_shortcuts_mac.mm:68: {true,  true,  false, false,
0,                'c', IDC_DEV_TOOLS_INSPECT},
How is this different than opt-cmd-i ? Should this have a menu bar entry? Most
things that have keyboard shortcuts are in the menu on mac (and the keyboard
shortcut is defined in MainMenu.xib). This list is merely for alternate
shortcuts.

[apavlov, 2010-07-15 17:09:48]

On 2010/07/15 17:02:29, Nico wrote:
> http://codereview.chromium.org/3011002/diff/2001/3012
> File chrome/browser/global_keyboard_shortcuts_mac.mm (right):
> 
> http://codereview.chromium.org/3011002/diff/2001/3012#newcode68
> chrome/browser/global_keyboard_shortcuts_mac.mm:68: {true,  true,  false,
false,
> 0,                'c', IDC_DEV_TOOLS_INSPECT},
> How is this different than opt-cmd-i ? Should this have a menu bar entry? Most
Developers want to be able to directly get into the "Inspect Element" mode
rather than have to switch panels and click a button in the DevTools window.
Please refer to the BUG=29532 - there's a lot of people asking for that (stars)
(and, there is a respective shortcut in FireBug - that's why).

> things that have keyboard shortcuts are in the menu on mac (and the keyboard
> shortcut is defined in MainMenu.xib). This list is merely for alternate
> shortcuts.
We think that having 3 menu entires for bringing up DevTools is an overkill.
Also, the whole point of this action is to have a shortcut for it. Hitting
cmd-opt-i, choosing the Elements panel and clicking a button seems faster than
getting through the menu hierarchy :) There is also no respective menu item for
Win and Linux.

[Nico, 2010-07-15 17:14:43]

Sounds reasonable. What's the shortcut for devtools on windows? ctrl-shift-i?
it's cmd-opt-i on mac; maybe the mac shortcut should be cmd-opt-c then?

[Nico, 2010-07-15 17:15:54]

Just read the bug. It's starred by a mere 20 people, and some of them seem to
misunderstand what the bug's about. I'm reluctant about adding this shortcut.

[Erik does not do reviews, 2010-07-15 17:18:56]

I would suggest that any new keyboard shortcuts need to go through UI
review.  Available shortcuts are in short supply and are used sparingly.  We
also need to consider conflicts and compatibility with other software.
 Please add UI-Needed to the bug and send mail to the ui leads.

Erik


On Thu, Jul 15, 2010 at 10:15 AM, <thakis@chromium.org> wrote:

> Just read the bug. It's starred by a mere 20 people, and some of them seem
> to
> misunderstand what the bug's about. I'm reluctant about adding this
> shortcut.
>
>
> http://codereview.chromium.org/3011002/show
>

[apavlov, 2010-07-15 20:52:21]

On 2010/07/15 17:15:54, Nico wrote:
> Just read the bug. It's starred by a mere 20 people, and some of them seem to
> misunderstand what the bug's about. I'm reluctant about adding this shortcut.

(a) I don't think only people on the thread star the bug, (b) these 20 people
make this bug a #1 FR for DevTools. I'll let Alexei (our PM, cc'ed) handle
Erik's proposal and other non-technical details.

[pfeldman, 2010-07-15 21:33:46]

More context for UI folks:
- Cmd (Control) - Shift - C was first used by FireBug. It is a shortcut to
variation of context menu's "Inspect Element". Variation in a sense that it
highlights an element that is going to be inspected right on the page.
- Same shortcut was then used by WebKit inspector to enter this 'inspect
element' mode. But it has been handled on inspector's elements panel only. So it
is available in Safari 5 and Chrome 5, but works from within inspector window
only (is handled locally using JavaScript).
- There is a general agreement that the shortcut should work from the inspected
page. Safari Web Inspector people support it. So it is not about adding a
shortcut - it is about widening its scope.
- In terms of adding it to the menu - I don't think we need it. We want it to be
a power user feature that is going to be poorly discoverable. People that were
using shortcut in FireBug will learn about it from one of our blog posts /
tutorials. Power users love shortcuts, DevTools customers are all power users.

So the point is that adding this two-modifiers shortcut is pretty much
legalizing its existence + we are aligned across browsers. If this does not
sound reasonable to the UI team, please respond. Otherwise we review and land
this. Thanks!

[Erik does not do reviews, 2010-07-15 22:15:57]

This all sounds reasonable to me, but I'm not on the UI team, and they're
not cc'd on this list.  I'm just suggesting you send mail to chrome-ui-leads
and let them comment on the bug.

Erik


On Thu, Jul 15, 2010 at 2:33 PM, <pfeldman@chromium.org> wrote:

> More context for UI folks:
> - Cmd (Control) - Shift - C was first used by FireBug. It is a shortcut to
> variation of context menu's "Inspect Element". Variation in a sense that it
> highlights an element that is going to be inspected right on the page.
> - Same shortcut was then used by WebKit inspector to enter this 'inspect
> element' mode. But it has been handled on inspector's elements panel only.
> So it
> is available in Safari 5 and Chrome 5, but works from within inspector
> window
> only (is handled locally using JavaScript).
> - There is a general agreement that the shortcut should work from the
> inspected
> page. Safari Web Inspector people support it. So it is not about adding a
> shortcut - it is about widening its scope.
> - In terms of adding it to the menu - I don't think we need it. We want it
> to be
> a power user feature that is going to be poorly discoverable. People that
> were
> using shortcut in FireBug will learn about it from one of our blog posts /
> tutorials. Power users love shortcuts, DevTools customers are all power
> users.
>
> So the point is that adding this two-modifiers shortcut is pretty much
> legalizing its existence + we are aligned across browsers. If this does not
> sound reasonable to the UI team, please respond. Otherwise we review and
> land
> this. Thanks!
>
>
> http://codereview.chromium.org/3011002/show
>

[pfeldman, 2010-07-15 22:17:31]

On 2010/07/15 22:15:57, Erik Kay wrote:
> This all sounds reasonable to me, but I'm not on the UI team, and they're
> not cc'd on this list.  I'm just suggesting you send mail to chrome-ui-leads
> and let them comment on the bug.
> 
> Erik

Oh, I thought list was already CC-ed. Thanks. Fixing.

[pfeldman, 2010-07-16 10:59:06]

LGTM.

I like this change. It is a good refactoring. Even if we get no go for the
Cmd+Shift+C - we should land the rest. I am suggesting that we land it as is now
and wait for UI people to get back to us in case they find it important enough.

http://codereview.chromium.org/3011002/diff/12001/13008
File chrome/browser/debugger/devtools_window.cc (right):

http://codereview.chromium.org/3011002/diff/12001/13008#newcode279
chrome/browser/debugger/devtools_window.cc:279: ExecuteJavascriptInWebFrame(L"",
L"WebInspector.showConsole();");
Add TODO that we should push it through the WebKit API.

[robertmarkbram, 2010-08-11 05:45:15]

I have build 6.0.472.25 dev on WinXP and Control+shift+c does nothing. Has it
been released yet?