Handle Escape key on constrained window.

Handle Escape key on constrained window.

Escape key is used to be handled by JS-side in each dialog implementation. This CL removes JS code and change to handle Escape key by C++-side in constrained window implementation globally.

This CL is based with based on http://crrev.com/127012 (reverted).

BUG=119684
TEST=manual, trybots pass on win and linux_chromeos

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

[yoshiki, 2012-05-30 06:15:41]

kochi: could you take a look?
estade: could you approve this?

[kochi, 2012-05-30 07:17:31]

Thanks for working on this, Yoshiki!

I commented some, but someone familiar with Aura code
should also review this.

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:130:
AcceleratorPressed(const ui::Accelerator& accelerator) OVERRIDE {
style nit: newline after open paren.
virtual bool AcceleratorPressed(
    const ui::Accelerator& accelerator) OVERRIDE {

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:136: };
nit: a newline after this line.

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:160: //
Pressing the ESC key will close the dialog.
probably "the" is not necessary.
You can reuse the same phrase on line 131.

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:162:
AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
Don't we need to RemoveAccelerator()?
(I'm not sure)

[yoshiki, 2012-05-30 08:43:15]

+sky: Could you take a look at constrained_web_dialog_delegate_views.cc?

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:130:
AcceleratorPressed(const ui::Accelerator& accelerator) OVERRIDE {
On 2012/05/30 07:17:31, Takayoshi Kochi wrote:
> style nit: newline after open paren.
> virtual bool AcceleratorPressed(
>     const ui::Accelerator& accelerator) OVERRIDE {

Done.

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:136: };
Other inherited methods are listed without blank line (above
views::WidgetDelegate interface methods). I think newline is not necessarily to
keep consistency.

On 2012/05/30 07:17:31, Takayoshi Kochi wrote:
> nit: a newline after this line.

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:160: //
Pressing the ESC key will close the dialog.
On 2012/05/30 07:17:31, Takayoshi Kochi wrote:
> probably "the" is not necessary.
> You can reuse the same phrase on line 131.

Done.

https://chromiumcodereview.appspot.com/10446076/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:162:
AddAccelerator(ui::Accelerator(ui::VKEY_ESCAPE, ui::EF_NONE));
I think it is not necessarily because WebDialogView class uses the same way but
has not RemoveAccelerator() (in chrome/browser/ui/views/web_dialog_view.cc).

On 2012/05/30 07:17:31, Takayoshi Kochi wrote:
> Don't we need to RemoveAccelerator()?
> (I'm not sure)

[yoshiki, 2012-05-30 08:43:48]

+sky: Could you take a look at constrained_web_dialog_delegate_views.cc?

[sky, 2012-05-30 14:59:48]

https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:15: #include
"ui/aura/event.h"
Only include aura if aura is defined.

https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:134:
GetWidget()->Close();
Don't you need to route this to CloseConstrainedWindow ?

[Evan Stade, 2012-05-30 21:05:47]

1) doesn't this break mac and linux?
2) am I correct in assuming that this still lets the page have a look at
"Escape", and only handles it if the page doesn't cancel propagation

[yoshiki, 2012-05-31 18:50:40]

sky: PTAL?

estade: Comments inline.
> 1) doesn't this break mac and linux?
You're right. I confirmed this breaks mac and linux. So I removed the
Javascript-side change in print_preview.js (Note that certificate_viewer.js
seems to be only used at ChromeOS). I'll add mac and linux C++ part at an
another CL.

> 2) am I correct in assuming that this still lets the page have a look at
> "Escape", and only handles it if the page doesn't cancel propagation
You are right too. But I think it is ok because this is same behavior as
WebDialog. What do you think?

https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:15: #include
"ui/aura/event.h"
On 2012/05/30 14:59:48, sky wrote:
> Only include aura if aura is defined.

Done.

https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:134:
GetWidget()->Close();
On 2012/05/30 14:59:48, sky wrote:
> Don't you need to route this to CloseConstrainedWindow ?

Done.

[Evan Stade, 2012-05-31 19:12:56]

lgtm

if you want, you can use grit to platform ifdef the escape handling in the print
preview code.

On 2012/05/31 18:50:40, yoshiki wrote:
> sky: PTAL?
> 
> estade: Comments inline.
> > 1) doesn't this break mac and linux?
> You're right. I confirmed this breaks mac and linux. So I removed the
> Javascript-side change in print_preview.js (Note that certificate_viewer.js
> seems to be only used at ChromeOS). I'll add mac and linux C++ part at an
> another CL.
> 
> > 2) am I correct in assuming that this still lets the page have a look at
> > "Escape", and only handles it if the page doesn't cancel propagation
> You are right too. But I think it is ok because this is same behavior as
> WebDialog. What do you think?

I think it's a good thing.

> 
>
https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
> File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
> chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:15: #include
> "ui/aura/event.h"
> On 2012/05/30 14:59:48, sky wrote:
> > Only include aura if aura is defined.
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10446076/diff/6001/chrome/browser/ui/v...
> chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:134:
> GetWidget()->Close();
> On 2012/05/30 14:59:48, sky wrote:
> > Don't you need to route this to CloseConstrainedWindow ?
> 
> Done.

[sky, 2012-05-31 21:38:23]

You'll need to verify this doesn't cause duplicating handling with print preview
on windows.

https://chromiumcodereview.appspot.com/10446076/diff/3004/chrome/browser/ui/v...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

https://chromiumcodereview.appspot.com/10446076/diff/3004/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:16: #include
"ui/views/widget/native_widget_aura.h"
This should only be included for aura too.

https://chromiumcodereview.appspot.com/10446076/diff/3004/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:24: #endif //
defined(USE_AURA)
remove the trailing //

https://chromiumcodereview.appspot.com/10446076/diff/3004/chrome/browser/ui/v...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:60: #if
defined(USE_AURA)
Can you use unhandled_keyboard_event_handler for this? That would make the code
non-aura specific.

[yoshiki, 2012-06-18 15:45:32]

sky: Sorry for late response. I confirmed Print Preview Dialog on Windows. PTAL

http://codereview.chromium.org/10446076/diff/3004/chrome/browser/ui/views/con...
File chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc (right):

http://codereview.chromium.org/10446076/diff/3004/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:16: #include
"ui/views/widget/native_widget_aura.h"
On 2012/05/31 21:38:23, sky wrote:
> This should only be included for aura too.

Done.

http://codereview.chromium.org/10446076/diff/3004/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:24: #endif //
defined(USE_AURA)
On 2012/05/31 21:38:23, sky wrote:
> remove the trailing //

Done.

http://codereview.chromium.org/10446076/diff/3004/chrome/browser/ui/views/con...
chrome/browser/ui/views/constrained_web_dialog_delegate_views.cc:60: #if
defined(USE_AURA)
On 2012/05/31 21:38:23, sky wrote:
> Can you use unhandled_keyboard_event_handler for this? That would make the
code
> non-aura specific.

Done.

[sky, 2012-06-18 16:58:14]

LGTM

[commit-bot: I haz the power, 2012-06-22 06:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoshiki@chromium.org/10446076/31001

[commit-bot: I haz the power, 2012-06-22 08:08:40]

Change committed as 143560