Support cmd-left/right for history.

Support cmd-left/right for history.

Since cmd-left/right should not do history if the omnibox is focussed, but cmd-1-9 should work if the omnibox is focussed, we have to differentiate between window- and browser-level shortcuts.

(Because performKeyEquivalent bubbles up from the window -- and if we let it bubble up to the omnibox, then the omnibox handles cmd-left/right just fine, but it swallows cmd-1 and doesn't give us a chance to intercept this. That means cmd-left doesn't work if you hit cmd-l tab, which focusses something that's neither omnibox nor tab contents. This behavior is consistent with safari and camino, and I think it's the best we can do without rewriting event dispatching.

Camino does this here:
http://mxr.mozilla.org/seamonkey/source/camino/src/browser/BrowserWindow.mm#128
http://mxr.mozilla.org/seamonkey/source/camino/src/browser/BrowserWrapper.mm#1031

)

BUG=12557
TEST=Focus text box on a web page. cmd-left/right should go to start/end of text. Focus webpage background. cmd-left/right \
should go history back/forward. When the omnibox is focussed, cmd-left/right should move the caret, but cmd-1-9 should still switch tabs.

Note that shortcuts still don't work if a subwindow (e.g. find bar, bookmark bubble) has focus.

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

[Nico, 2009-10-08 18:07:56]

Turns out this is somewhat independent of issue 15090 after all.

[jeremy, 2009-10-08 19:43:57]

Thanks for taking this on Nico!

Code looks fine.

My main concern here is to have this well documented since our keyboard handling
keypaths are a bit hard to grok.

Also, it goes without saying but nits are only nits, feel free to ignore them.

http://codereview.chromium.org/251069/diff/6006/6007
File chrome/browser/cocoa/chrome_browser_window.h (right):

http://codereview.chromium.org/251069/diff/6006/6007#newcode18
Line 18: - (BOOL)handleExtraWindowKeyboardShortcut:(NSEvent*)event;
Could you provide some comments from this function.

http://codereview.chromium.org/251069/diff/6006/6008
File chrome/browser/cocoa/chrome_browser_window.mm (right):

http://codereview.chromium.org/251069/diff/6006/6008#newcode15
Line 15: (int(*)(bool, bool, bool, int))commandForKeyboardShortcut {
Could you typedef the FP here, which should make this a little more readable.

http://codereview.chromium.org/251069/diff/6006/6008#newcode53
Line 53: if ([r isKindOfClass:[RenderWidgetHostViewCocoa class]])
Stupid question: Are we sure web content can't be present in any other kind of
Cocoa view?  Any way of making this check more robust?
The answer to both of these may be "no" but I just wanted to see if we can be
more defensive here.

http://codereview.chromium.org/251069/diff/6006/6008#newcode58
Line 58: // if e.g. the omnibox was focussed).
*was focussed -> has focus
*omnibox ->Omnibox

http://codereview.chromium.org/251069/diff/6006/6009
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/251069/diff/6006/6009#newcode23
Line 23: // browser is focussed.
Could you expand on this a bit? esp. browser vs window level shortcuts.

If you decide to document this elsewhere that's fine but could you point to it
from here.

*focused

http://codereview.chromium.org/251069/diff/6006/6009#newcode34
Line 34: const KeyboardShortcutData* GetWindowKeyboardShortCutTable(size_t*
num_entries);
*Shortcut

http://codereview.chromium.org/251069/diff/6006/6011
File chrome/browser/global_keyboard_shortcuts_mac_unittest.cc (right):

http://codereview.chromium.org/251069/diff/6006/6011#newcode13
Line 13: ASSERT_EQ(-1, CommandForWindowKeyboardShortcut(false, false, false,
0));
nit: 0 == func() always seemed confusing to me, could you change back the order
of the arguments to ASSERT_EQ.

http://codereview.chromium.org/251069/diff/6006/6011#newcode35
Line 35: TEST(GlobalKeyboardShortcuts, ShortCutsToBrowserCommand) {
*Shortcuts

[Nico, 2009-10-08 20:16:52]

Thanks for the quick review!

http://codereview.chromium.org/251069/diff/6006/6008
File chrome/browser/cocoa/chrome_browser_window.mm (right):

http://codereview.chromium.org/251069/diff/6006/6008#newcode15
Line 15: (int(*)(bool, bool, bool, int))commandForKeyboardShortcut {
On 2009/10/08 19:43:57, jeremy wrote:
> Could you typedef the FP here, which should make this a little more readable.

:-)

http://codereview.chromium.org/251069/diff/6006/6008#newcode53
Line 53: if ([r isKindOfClass:[RenderWidgetHostViewCocoa class]])
On 2009/10/08 19:43:57, jeremy wrote:
> Stupid question: Are we sure web content can't be present in any other kind of
> Cocoa view?  Any way of making this check more robust?
> The answer to both of these may be "no" but I just wanted to see if we can be
> more defensive here.

Excellent question. The only other place that I know of that shows web contents
is extensions, and those use RWHVCs too.

http://codereview.chromium.org/251069/diff/6006/6008#newcode58
Line 58: // if e.g. the omnibox was focussed).
On 2009/10/08 19:43:57, jeremy wrote:
> *was focussed -> has focus
> *omnibox ->Omnibox

Done.

http://codereview.chromium.org/251069/diff/6006/6009
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/251069/diff/6006/6009#newcode23
Line 23: // browser is focussed.
On 2009/10/08 19:43:57, jeremy wrote:
> Could you expand on this a bit? esp. browser vs window level shortcuts.
> 
> If you decide to document this elsewhere that's fine but could you point to it
> from here.
> 
> *focused

Done.

http://codereview.chromium.org/251069/diff/6006/6009#newcode34
Line 34: const KeyboardShortcutData* GetWindowKeyboardShortCutTable(size_t*
num_entries);
On 2009/10/08 19:43:57, jeremy wrote:
> *Shortcut

Done.

http://codereview.chromium.org/251069/diff/6006/6011
File chrome/browser/global_keyboard_shortcuts_mac_unittest.cc (right):

http://codereview.chromium.org/251069/diff/6006/6011#newcode13
Line 13: ASSERT_EQ(-1, CommandForWindowKeyboardShortcut(false, false, false,
0));
On 2009/10/08 19:43:57, jeremy wrote:
> nit: 0 == func() always seemed confusing to me, could you change back the
order
> of the arguments to ASSERT_EQ.

I don't like "0 == func()" at all, but the argument order is "expected, actual"
for ASSERT_EQ (cf.
http://code.google.com/p/googletest/wiki/GoogleTestPrimer#Binary_Comparison ).
This might print "Expected foo, got bar", and if the ordering is wrong, the
message will be confusing.

http://codereview.chromium.org/251069/diff/6006/6011#newcode35
Line 35: TEST(GlobalKeyboardShortcuts, ShortCutsToBrowserCommand) {
On 2009/10/08 19:43:57, jeremy wrote:
> *Shortcuts

Done.

[jeremy, 2009-10-08 21:25:33]

LGTM :)

http://codereview.chromium.org/251069/diff/10013/11003
File chrome/browser/cocoa/chrome_browser_window.h (right):

http://codereview.chromium.org/251069/diff/10013/11003#newcode18
Line 18: // See global_keyboard_shortcuts_mac.h for details on these.
The one important thing I'd expect to see in this comment which isn't
immediately obvious is if these functions do any work.  Could you make that
clear in the comment itself?

http://codereview.chromium.org/251069/diff/10013/11005
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/251069/diff/10013/11005#newcode20
Line 20: // was no match.
Could you add a comment to point back to the bug you're fixing here, that should
hopefully help understanding the split bellow.

Better yet, the comment you have for the code review is great and could help
here.

[Nico, 2009-10-08 23:55:49]

Thanks. I will go ahead and submit this, I hope the comments are now clear
enough :-)

http://codereview.chromium.org/251069/diff/10013/11003
File chrome/browser/cocoa/chrome_browser_window.h (right):

http://codereview.chromium.org/251069/diff/10013/11003#newcode18
Line 18: // See global_keyboard_shortcuts_mac.h for details on these.
On 2009/10/08 21:25:34, jeremy wrote:
> The one important thing I'd expect to see in this comment which isn't
> immediately obvious is if these functions do any work.  Could you make that
> clear in the comment itself?

Done.

http://codereview.chromium.org/251069/diff/10013/11005
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/251069/diff/10013/11005#newcode20
Line 20: // was no match.
On 2009/10/08 21:25:34, jeremy wrote:
> Could you add a comment to point back to the bug you're fixing here, that
should
> hopefully help understanding the split bellow.
> 
> Better yet, the comment you have for the code review is great and could help
> here.

Took parts from the CL description.

[pink (ping after 24hrs), 2009-10-09 14:36:51]

drive-by.

http://codereview.chromium.org/251069/diff/8010/7015
File chrome/browser/cocoa/chrome_browser_window.mm (right):

http://codereview.chromium.org/251069/diff/8010/7015#newcode54
Line 54: NSResponder* r = [self firstResponder];
what happens when the renderer is hung? Does that mean that these keys no longer
work? I remember discussion about how much this sucks on windows a month or so
ago.

http://codereview.chromium.org/251069/diff/8010/7016
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/251069/diff/8010/7016#newcode25
Line 25: // to intercept this. Hence, we need to types of keyboard shortcuts.
s/to/two

[Nico, 2009-10-09 16:50:06]

http://codereview.chromium.org/251069/diff/8010/7015
File chrome/browser/cocoa/chrome_browser_window.mm (right):

http://codereview.chromium.org/251069/diff/8010/7015#newcode54
Line 54: NSResponder* r = [self firstResponder];
On 2009/10/09 14:36:51, pink wrote:
> what happens when the renderer is hung? Does that mean that these keys no
longer
> work? I remember discussion about how much this sucks on windows a month or so
> ago.

Yes, that will work once BrowserWindowCocoa::GetCommandId() is implemented (part
of issue 15090).

What will happen (but doesn't yet) is that the rwhvmac will not send the key to
the renderer in this case.

See RenderWidgetHost::ForwardKeyboardEvent(): it calls  ShouldSendToRenderer()
to decide if it should ipc the key, which ends up calling
Browser::IsReservedAccelerator(), which returns true if the accelerator has a
command id of IDC_SELECT_NEXT_TAB etc.

So BrowserWindowCocoa::GetCommandId() only has to look at
global_keyboard_shortcuts_mac and all should be fine.

(Having said that, jam's patch seems to have problems on linux with
international keyboard layouts, so maybe it will be backed out again. Who
knows.)

http://codereview.chromium.org/251069/diff/8010/7016
File chrome/browser/global_keyboard_shortcuts_mac.h (right):

http://codereview.chromium.org/251069/diff/8010/7016#newcode25
Line 25: // to intercept this. Hence, we need to types of keyboard shortcuts.
On 2009/10/09 14:36:51, pink wrote:
> s/to/two

D'oh. ne.