Implement HandleKeyboardEvent() method in HtmlDialog implementations....

Implement HandleKeyboardEvent() method in HtmlDialog implementations.

To send unhandled keyboard events to the system default handler, so that system accelerators can work as normal.

BUG=30707
: Keyboard shortcuts failed to work on Bookmark sync Sign in Authentication dialog
TEST=See bug report.


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

[James Su, 2010-01-08 11:13:17]

I just tested this CL on Windows, which works as expected. And I found that
Linux port also needs similar fix.

Regards
James Su

[akalin, 2010-01-08 21:53:05]

LGTM

Although I'm curious; the {Pre,}HandleKeyboardEvent() functions in the various
platform BrowserWindow subclasses seem to have a lot more logic than what you
put in here, even after removing the browser-related stuff, and yet everything
seems to work fine.  Why is that?

On 2010/01/08 11:13:17, James Su wrote:
> I just tested this CL on Windows, which works as expected. And I found that
> Linux port also needs similar fix.
> 
> Regards
> James Su

[Nico, 2010-01-08 22:00:04]

LG

I have the same question as akalin :-)

http://codereview.chromium.org/525112/diff/2001/2004
File chrome/browser/cocoa/html_dialog_window_controller.mm (right):

http://codereview.chromium.org/525112/diff/2001/2004#newcode180
chrome/browser/cocoa/html_dialog_window_controller.mm:180: [event_window
redispatchEvent:event.os_event];
This means that all the shortcuts in global_keyboard_shortcuts_mac.mm won't
work. That's fine atm since they are all browser-specific, but it feels like it
could break stuff in the future.

[akalin (wrong akalin), 2010-01-08 22:05:42]

On Fri, Jan 8, 2010 at 2:00 PM,  <thakis@chromium.org> wrote:
> LG
>
> I have the same question as akalin :-)
>
> http://codereview.chromium.org/525112/diff/2001/2004#newcode180
> chrome/browser/cocoa/html_dialog_window_controller.mm:180: [event_window
> redispatchEvent:event.os_event];
> This means that all the shortcuts in global_keyboard_shortcuts_mac.mm
> won't work. That's fine atm since they are all browser-specific, but it
> feels like it could break stuff in the future.

Ah, this partially answers my question.

Perhaps add comments explaining the situation in the newly-added
functions?  something like:

// A simplified version of
BrowserWindowCocoa::HandleKeyboardShortcut().  We don't handle global
keyboard shortcuts here, but that's fine since they're all
browser-specific. (This may change in the future.)

and analogous for the other two platforms.

-- 
Frederick Akalin
Software Engineer