Fix BrowserKeyEventsTest.ReservedAccelerators on Mac 10.6

Fix BrowserKeyEventsTest.ReservedAccelerators on Mac 10.6

There are two reasons that cause this problem:
1. See issue http://crbug.com/65375
2. By some reason, when BrowserWindowCocoa::PreHandleKeyboardEvent() is
called for Cmd-W, it fails to get its corresponding command id, because
the corresponding menu item is not enabled at that time.

This CL does following changes:
1. Change testing flow to work around issue 65375.
2. Fix the second problem by always retrieving the corresponding command
id regardless of the menu item's state.

The reason of the second issue:
When Cmd-T gets handled in NSApp, BrowserWindowController's -validateUserInterfaceItem: method will be called for all menu items to determine their enable states and the states will be cached in each menu item. At this time, because there is only one tab, the "Close Tab" menu item will be disabled.
Then when sending Cmd-W to the browser, it'll firstly be handled in BrowserWindowCocoa::PreHandleKeyboardEvent() method, which will look up the
command id associated to Cmd-W, but at this time, BrowserWindowController's -validateUserInterfaceItem: method does not get called yet, so the corresponding menu item is still not enabled. Then BrowserWindowCocoa::PreHandleKeyboardEvent() will not handle Cmd-W because it
cannot find its command id.

After removing the check in cr_firesForKeyEvent method, BrowserWindowCocoa::PreHandleKeyboardEvent() method will always get the correct command id of a key equivalent regardless of its enable state, then if it's a reserved key equivalent, it will always be sent to NSApp for handling, which will call BrowserWindowController's -validateUserInterfaceItem: to update the enable states of all menu items. Then if the corresponding menu item is really disabled, NSApp will do nothing and return false, then the key event will be dispatched to web page as normal.

BUG=50447
BrowserKeyEventsTest.ReservedAccelerators failed on Mac10.6 Tests (dbg)(1) bot.
TEST=The test should not flaky.

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

[James Su, 2010-12-04 01:55:57]

I'm trying to write a test for issue 65375, but seems that it's hard to
reproduce in a test because of timing issue.

[Ilya Sherman, 2010-12-04 02:31:00]

There is a lot of code that is reproduced in the Windows and Mac portions of the
test.  Could we share more of it, with more narrow use of #ifdefs?

The NSMenuItem change worries me.  It fixes the problem in this test, but I'm
not convinced that it will behave as we want for other clients of the method. 
Nico probably knows better about that.

I would suggest, though, using test_timeouts::action_timeout_ms() here for now;
and filling a follow-up bug to track down why the menu item is disabled when we
think it should be enabled.

On 2010/12/04 01:55:57, James Su wrote:
> I'm trying to write a test for issue 65375, but seems that it's hard to
> reproduce in a test because of timing issue.

Is it that you don't see failures even if you run it in a tight loop; or that
you do see failures, but not consistently?

http://codereview.chromium.org/5576002/diff/5001/chrome/browser/browser_keyev...
File chrome/browser/browser_keyevents_browsertest.cc (right):

http://codereview.chromium.org/5576002/diff/5001/chrome/browser/browser_keyev...
chrome/browser/browser_keyevents_browsertest.cc:688: // Press Cmd+T, which will
open a new tab, it can not be suppressed.
nit: The added part would be better as ". It cannot be suppressed." (period
rather than comma, no space between "can" and "not")

[James Su, 2010-12-04 03:21:11]

For now, I'd rather to keep code of different platforms separated, which is
clearer. As they have different testing flows.

For the NSMenuItem change, it should be pretty safe, as it's only used in
BrowserWindowCocoa::GetCommandId() method to get the command id of a key
equivalent. And it actually makes no sense to check the enable state there,
because the enable state is actually determined by calling
BrowserWindowController's -validateUserInterfaceItem: method, just before a key
equivalent is being handled in NSApp.

The reason that why it causes the test failure is: when Cmd-T gets handled in
NSApp, BrowserWindowController's -validateUserInterfaceItem: method will be
called for all menu items to determine their enable states and the states will
be cached in each menu item. At this time, because there is only one tab, the
"Close Tab" menu item will be disabled.
Then when sending Cmd-W to the browser, it'll firstly be handled in
BrowserWindowCocoa::PreHandleKeyboardEvent() method, which will look up the
command id associated to Cmd-W, but at this time, BrowserWindowController's
-validateUserInterfaceItem: method does not get called yet, so the corresponding
menu item is still not enabled. Then
BrowserWindowCocoa::PreHandleKeyboardEvent() will not handle Cmd-W because it
cannot find its command id.
After removing the check in cr_firesForKeyEvent method,
BrowserWindowCocoa::PreHandleKeyboardEvent() method will always get the correct
command id of a key equivalent regardless of its enable state, then if it's a
reserved key equivalent, it will always be sent to NSApp for handling, which
will call BrowserWindowController's -validateUserInterfaceItem: to update the
enable states of all menu items. Then if the corresponding menu item is really
disabled, NSApp will do nothing and return false, then the key event will be
dispatched to web page as normal.

For issue 65375, I wrote a test to try to reproduce it, but the test didn't fail
as expected. Even I ran it with --gtest_repeat=-1 for several minutes. Although
I can reproduce this issue fairly consistently by hand.

The reason that why the original test is flaky instead of consistently failing
is: when the test passed, it's just because the focus is in omnibox and Cmd-W
just worked there.

http://codereview.chromium.org/5576002/diff/5001/chrome/browser/browser_keyev...
File chrome/browser/browser_keyevents_browsertest.cc (right):

http://codereview.chromium.org/5576002/diff/5001/chrome/browser/browser_keyev...
chrome/browser/browser_keyevents_browsertest.cc:688: // Press Cmd+T, which will
open a new tab, it can not be suppressed.
On 2010/12/04 02:31:00, Ilya Sherman wrote:
> nit: The added part would be better as ". It cannot be suppressed." (period
> rather than comma, no space between "can" and "not")

Done.

[Nico, 2010-12-04 20:25:14]

LG

Thanks for tracking this down, and for the great answer to Ilya's question –
consider moving some of that into the CL description.

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
File chrome/browser/ui/cocoa/nsmenuitem_additions.h (right):

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
chrome/browser/ui/cocoa/nsmenuitem_additions.h:20: -
(BOOL)cr_firesForKeyEvent:(NSEvent*)event;
Can you leave the function as is and instead add a new function
cr_firesForKeyEventIfEnabled: that has the new semantics?

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
File chrome/browser/ui/cocoa/nsmenuitem_additions.mm (right):

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
chrome/browser/ui/cocoa/nsmenuitem_additions.mm:13: -
(BOOL)cr_firesForKeyEvent:(NSEvent*)event {
You can then implement this function as

 … {
  if (![self isEnabled])
    return NO;
  return [cr_firesForKeyEventIfEnabled:event];
}

[James Su, 2010-12-06 18:37:28]

CL updated.

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
File chrome/browser/ui/cocoa/nsmenuitem_additions.h (right):

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
chrome/browser/ui/cocoa/nsmenuitem_additions.h:20: -
(BOOL)cr_firesForKeyEvent:(NSEvent*)event;
On 2010/12/04 20:25:15, Nico wrote:
> Can you leave the function as is and instead add a new function
> cr_firesForKeyEventIfEnabled: that has the new semantics?

Done.

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
File chrome/browser/ui/cocoa/nsmenuitem_additions.mm (right):

http://codereview.chromium.org/5576002/diff/7003/chrome/browser/ui/cocoa/nsme...
chrome/browser/ui/cocoa/nsmenuitem_additions.mm:13: -
(BOOL)cr_firesForKeyEvent:(NSEvent*)event {
On 2010/12/04 20:25:15, Nico wrote:
> You can then implement this function as
> 
>  … {
>   if (![self isEnabled])
>     return NO;
>   return [cr_firesForKeyEventIfEnabled:event];
> }

Done.

[Ilya Sherman, 2010-12-06 18:45:27]

LGTM

http://codereview.chromium.org/5576002/diff/12002/chrome/browser/ui/cocoa/nsm...
File chrome/browser/ui/cocoa/nsmenuitem_additions.mm (right):

http://codereview.chromium.org/5576002/diff/12002/chrome/browser/ui/cocoa/nsm...
chrome/browser/ui/cocoa/nsmenuitem_additions.mm:13: -
(BOOL)cr_firesForKeyEvent:(NSEvent*)event {
nit: It seems weird to me to leave this function around without any clients. 
Nico thinks it might be useful for clients outside of Chromium though.  *shrug*