Handle keyboard shortcuts in Mac Panels.

Handle keyboard shortcuts in Mac Panels.

Common code from browser_window_cocoa separated into cocoa/browser_window_utils.

Panel.xib changed to make the NSWindow for the PanelWindowControllerCocoa a ChromeEventProcessingWindow.

BUG=None
TEST=Manual testing with Panels and keyboard shortcuts to find/next/previous/close window/quit chrome.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98112 (Reverted by http://codereview.chromium.org/7747011/)

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

[jennb, 2011-08-24 01:02:12]

Dmitry for Panels, Avi for Cocoa expertise.

Did not add a new keyboard shortcut test as existing ones are disabled for Mac.
There isn't a platform-independent Panels test for keyboard shortcuts because I
couldn't find a way to send a key event to the Panel. I can only manage a key
event being sent directly to the native window layer
(ui_test_utils::SendKeyPressSync), which wouldn't exercise any of this keyboard
handling logic.

[Avi (use Gerrit), 2011-08-24 05:01:10]

LGTM with nits.

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_utils.h (right):

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.h:17: +
(BOOL)shouldHandleKeyboardEvent:(const NativeWebKeyboardEvent&)event;
Passing ObjC parameters by const&? Wow, I guess that works (this is ObjC++) but
do we do that elsewhere? Part of me wants to make you change it, part says, hey,
cool, this works.

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.h:28: @interface MenuWalker :
NSObject
MenuWalker appears to be an implementation detail of this class. Move the
declaration to the .mm file.

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.mm:100: for (NSMenuItem *item in
[menu itemArray]) {
Move * to the left.

[jennb, 2011-08-24 17:50:33]

Thanks for the quick look, Avi!

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_utils.h (right):

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.h:17: +
(BOOL)shouldHandleKeyboardEvent:(const NativeWebKeyboardEvent&)event;
On 2011/08/24 05:01:10, Avi wrote:
> Passing ObjC parameters by const&? Wow, I guess that works (this is ObjC++)
but
> do we do that elsewhere? Part of me wants to make you change it, part says,
hey,
> cool, this works.

Amazing what people do when they don't know what's impossible. :-)  After your
comment, I went looking and actually found some examples of this, one of which I
traced back to thakis in tab_contents_view_mac.mm and one which seems to trace
back to you in app_controller_mac.mm. After those two, I stopped looking as
you're both on my short list of Cocoa experts.

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.h:28: @interface MenuWalker :
NSObject
On 2011/08/24 05:01:10, Avi wrote:
> MenuWalker appears to be an implementation detail of this class. Move the
> declaration to the .mm file.

Done.

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_utils.mm (right):

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.mm:100: for (NSMenuItem *item in
[menu itemArray]) {
On 2011/08/24 05:01:10, Avi wrote:
> Move * to the left.

Done.

[Dmitry Titov, 2011-08-24 17:53:17]

Yay! Common functionality between Panels and BWC!
LGTM for panels.

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
File chrome/browser/ui/cocoa/browser_window_utils.h (right):

http://codereview.chromium.org/7719016/diff/1/chrome/browser/ui/cocoa/browser...
chrome/browser/ui/cocoa/browser_window_utils.h:17: +
(BOOL)shouldHandleKeyboardEvent:(const NativeWebKeyboardEvent&)event;
On 2011/08/24 05:01:10, Avi wrote:
> Passing ObjC parameters by const&? Wow, I guess that works (this is ObjC++)
but
> do we do that elsewhere? Part of me wants to make you change it, part says,
hey,
> cool, this works.

Interesting indeed. I'm no ObjC++ expert, but unless we want to amend current
style rules, usually consistency wins. This way, people don't need to guess
which one to use. Unless we change the rules of course :-)

[jennb, 2011-08-26 06:18:57]

Uploaded a new revision restoring the 2 lines I accidentally deleted when I
moved the code to browser_window_utils.mm - thanks, Dmitry for finding it! 
Re-ran the try job and interactive_ui_tests now pass.  The try job failed when I
ran it without these two lines.

Do you want to take another look before I commit again?

[Dmitry Titov, 2011-08-26 06:32:57]

LGTM

[Avi (use Gerrit), 2011-08-26 12:25:42]

LGTM

On Fri, Aug 26, 2011 at 2:32 AM, <dimich@chromium.org> wrote:

> LGTM
>
>
>
http://codereview.chromium.**org/7719016/<http://codereview.chromium.org/7719...
>