Added an event handler to the GAIA login page which closes the dialog when it...

Added an accelerator to the HtmlDialogView class so that hitting the ESC key closes the dialog.

BUG=19786
TEST=Open Sync Bookmaeks Login dialog. Enter credentials, TAB between fields, etc. to ensure nothing is broken. Hit the ESC key to dismiss the dialog.

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

[RandyPosynick, 2009-09-09 22:33:42]

I have been following the comments in bug 19896 and realize that this dialog is
probably going to be changed, but you may wish to use this regardless.

[tim (not reviewing), 2009-09-10 15:15:39]

Thanks for doing this!

http://codereview.chromium.org/199069/diff/1/2
File chrome/browser/sync/resources/gaia_login.html (right):

http://codereview.chromium.org/199069/diff/1/2#newcode103
Line 103: if (e && e.which == 27) {
The thing with this approach is it doesn't help any other HTML dialogs (albeit
we only have one other); each one has to implement it themselves, but in reality
they all want this functionality because they are dialogs (e.g Create
Application Shortcuts dialog).  In that one, the HTML isn't even in the chromium
repository so doing this is kinda tricky.  Did you look at any other way to do
this at a higher / c++ level?  I'm curious what would happen if HtmlDialogView
used View::AddAccelerator, and overrode AcceleratorPressed to close itself on
VK_ESCAPE.

[RandyPosynick, 2009-09-10 18:33:52]

Sure, I can take a look at doing this generally for all HTML dialogs.

[RandyPosynick, 2009-09-10 20:44:33]

All right, the latest patch adds an accelerator to the HtmlDialogView class, to
handle this case for all HTML dialogs.

[tim (not reviewing), 2009-09-10 21:21:31]

LGTM with a couple nits fixed.

This is your first checkin, correct?  We'll need to determine which licensing
agreement you need to sign (see
http://dev.chromium.org/developers/contributing-code), and add names to the
AUTHORS file before checking this in.

http://codereview.chromium.org/199069/diff/5001/6002
File chrome/browser/views/html_dialog_view.cc (right):

http://codereview.chromium.org/199069/diff/5001/6002#newcode52
Line 52: // ESC closes the dialog
nit - add period.

http://codereview.chromium.org/199069/diff/5001/6002#newcode53
Line 53: DCHECK(accelerator.GetKeyCode() == base::VKEY_ESCAPE);
nit - prefer DCHECK_EQ(base::VKEY_ESCAPE, accelerator.GetKeyCode())

http://codereview.chromium.org/199069/diff/5001/6002#newcode54
Line 54: OnDialogClosed("");
nit- prefer std::string().

http://codereview.chromium.org/199069/diff/5001/6002#newcode211
Line 211: // Pressing the ESC key will close the dialog
nit - add period.

[RandyPosynick, 2009-09-25 21:53:20]

Addressed nits and modified AUTHORS file.

[tim (not reviewing), 2009-09-28 17:19:40]

On 2009/09/25 21:53:20, RandyPosynick wrote:
> Addressed nits and modified AUTHORS file.

LGTM! Committed as r27371. Congrats on the first patch!