cros: Bind hangup red button on remote controller to close web dialog

cros: Bind hangup red button on remote controller to close web dialog

Changes:
The hangup red button on remote controller is mapped to 'Shift + Browser Back (F1)'. Add this accelerator to allow closing web dialog. 

BUG=700128
TEST=tested with hotrod remote controller
and added test coverage

Review-Url: https://codereview.chromium.org/2871073002
Cr-Commit-Position: refs/heads/master@{#471864}
Committed: https://chromium.googlesource.com/chromium/src/+/9e608edc70f96a0d9ce6aef52d88928d851a7fe8

[Qiang(Joe) Xu, 2017-05-10 02:31:20]

Description was changed from

==========
cros: Bind 'Shift+Backspace' to close login web dialog

BUG=700128
TEST=tested with hotrod remote controller
==========

to

==========
cros: Bind 'Shift+Backspace' to close login web dialog

Changes:
This is not-mus-ready solution of providing a key-based way to close the dialog
since we cannot move the focus to the non client area [x] button. 

BUG=700128
TEST=tested with hotrod remote controller
==========

[Qiang(Joe) Xu, 2017-05-10 02:31:21]

warx@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Qiang(Joe) Xu, 2017-05-10 02:32:52]

Hi xiyuan, please take a look first, thanks!

[Qiang(Joe) Xu, 2017-05-10 02:51:39]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 02:52:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 05:06:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 05:06:53]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-05-10 16:48:56]

Can we extend WebDialogDelegate interface to
- provide additional accelerators to WebDialogView;
- have a AccleratorPressed to handle them (from WebDialogView)

?

This way, it should work in all env and not depend on pre-target handler.

If we want to go the pre-target handler approach, we should use
ShellPort::CreateKeyEventWatcher and use that to get the key. This way does not
support mash either.

[Qiang(Joe) Xu, 2017-05-10 19:17:51]

Description was changed from

==========
cros: Bind 'Shift+Backspace' to close login web dialog

Changes:
This is not-mus-ready solution of providing a key-based way to close the dialog
since we cannot move the focus to the non client area [x] button. 

BUG=700128
TEST=tested with hotrod remote controller
==========

to

==========
cros: Bind hangup red button on remote controller to close web dialog

Changes:
The hangup red button on remote controller is mapped to 'Shift + Browser Back
(F1)'. Add this accelerator to allow closing web dialog. 

BUG=700128
TEST=tested with hotrod remote controller
==========

[Qiang(Joe) Xu, 2017-05-10 19:26:10]

On 2017/05/10 16:48:56, xiyuan wrote:
> Can we extend WebDialogDelegate interface to
> - provide additional accelerators to WebDialogView;
> - have a AccleratorPressed to handle them (from WebDialogView)
> 
> ?
> 
> This way, it should work in all env and not depend on pre-target handler.
> 
> If we want to go the pre-target handler approach, we should use
> ShellPort::CreateKeyEventWatcher and use that to get the key. This way does
not
> support mash either.

Hi xiyuan, your first suggestion is a way to add accelerator only on
login_web_dialog.
How about we just add another accelerator 'Shift + Browser Back(F1)' to close
the web dialog?
It is mapped to the red button (hangup) on hotrod remote controller.

I don't see shift+f1 is used on chrome os. So, this would be properly fine.

[xiyuan, 2017-05-10 19:43:00]

On 2017/05/10 19:26:10, Qiang(Joe) Xu wrote:
> On 2017/05/10 16:48:56, xiyuan wrote:
> > Can we extend WebDialogDelegate interface to
> > - provide additional accelerators to WebDialogView;
> > - have a AccleratorPressed to handle them (from WebDialogView)
> > 
> > ?
> > 
> > This way, it should work in all env and not depend on pre-target handler.
> > 
> > If we want to go the pre-target handler approach, we should use
> > ShellPort::CreateKeyEventWatcher and use that to get the key. This way does
> not
> > support mash either.
> 
> Hi xiyuan, your first suggestion is a way to add accelerator only on
> login_web_dialog.
> How about we just add another accelerator 'Shift + Browser Back(F1)' to close
> the web dialog?
> It is mapped to the red button (hangup) on hotrod remote controller.
> 
> I don't see shift+f1 is used on chrome os. So, this would be properly fine.

We need views OWNER to buy into the idea. web_dialog_view.cc is shared code
running on all platforms. Having a CfM/ChromeOS specifics there is probably not
to get approved. I prefer to extend the interface so that we only add this
accelerator for LoginWebDialog so that the scope is limited.

[Qiang(Joe) Xu, 2017-05-10 21:00:27]

On 2017/05/10 19:43:00, xiyuan wrote:
> On 2017/05/10 19:26:10, Qiang(Joe) Xu wrote:
> > On 2017/05/10 16:48:56, xiyuan wrote:
> > > Can we extend WebDialogDelegate interface to
> > > - provide additional accelerators to WebDialogView;
> > > - have a AccleratorPressed to handle them (from WebDialogView)
> > > 
> > > ?
> > > 
> > > This way, it should work in all env and not depend on pre-target handler.
> > > 
> > > If we want to go the pre-target handler approach, we should use
> > > ShellPort::CreateKeyEventWatcher and use that to get the key. This way
does
> > not
> > > support mash either.
> > 
> > Hi xiyuan, your first suggestion is a way to add accelerator only on
> > login_web_dialog.
> > How about we just add another accelerator 'Shift + Browser Back(F1)' to
close
> > the web dialog?
> > It is mapped to the red button (hangup) on hotrod remote controller.
> > 
> > I don't see shift+f1 is used on chrome os. So, this would be properly fine.
> 
> We need views OWNER to buy into the idea. web_dialog_view.cc is shared code
> running on all platforms. Having a CfM/ChromeOS specifics there is probably
not
> to get approved. I prefer to extend the interface so that we only add this
> accelerator for LoginWebDialog so that the scope is limited.

Hi xiyuan, ps3 is a discussion only patch. As I feel the API is pretty ugly. Any
suggestions?

[xiyuan, 2017-05-10 21:14:56]

https://codereview.chromium.org/2871073002/diff/40001/ui/web_dialogs/web_dial...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/2871073002/diff/40001/ui/web_dialogs/web_dial...
ui/web_dialogs/web_dialog_delegate.h:143: virtual bool
AdditionalAccelerator(const ui::Accelerator& accelerator) const {
I am thinking of something like this:

virtual std::vector<ui::Accelerator> GetAccelerators();
virtual bool AcceleratorPressed(const Accelerator& accelerator);

LoginWebDialog can use this to provide an accelerate and bind that with close.

[Qiang(Joe) Xu, 2017-05-10 21:18:36]

On 2017/05/10 21:14:56, xiyuan wrote:
>
https://codereview.chromium.org/2871073002/diff/40001/ui/web_dialogs/web_dial...
> File ui/web_dialogs/web_dialog_delegate.h (right):
> 
>
https://codereview.chromium.org/2871073002/diff/40001/ui/web_dialogs/web_dial...
> ui/web_dialogs/web_dialog_delegate.h:143: virtual bool
> AdditionalAccelerator(const ui::Accelerator& accelerator) const {
> I am thinking of something like this:
> 
> virtual std::vector<ui::Accelerator> GetAccelerators();
> virtual bool AcceleratorPressed(const Accelerator& accelerator);
> 
> LoginWebDialog can use this to provide an accelerate and bind that with close.

Cool, sounds promising, thanks!

[Qiang(Joe) Xu, 2017-05-10 22:40:21]

Hi xiyuan, PTAL thanks

+sky for c/b/u and ui/ owner review

After https://codereview.chromium.org/2875513003/ is landed, I will add test
coverage for this. You can also hold off review until I update the patch again.
Thanks!

[Qiang(Joe) Xu, 2017-05-10 22:40:52]

warx@chromium.org changed reviewers:
+ sky@chromium.org

[xiyuan, 2017-05-11 04:48:27]

Mostly good. Let me know when I should take another look.

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_web_dialog.cc (right):

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:64:
ui::Accelerator(ui::VKEY_BROWSER_BACK, ui::EF_SHIFT_DOWN));
Move this into GetAccelerators and get rid of |accelerators_| member.

[sky, 2017-05-11 15:04:11]

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_web_dialog.cc (right):

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:74: dialog_window_ =
chrome::ShowWebDialogInContainer(
Initialize dialog_window_ to null.

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:185: if
(accelerator.key_code() == ui::VKEY_BROWSER_BACK &&
Have a function that you use to get the accelerator so that you don't need to
duplicate it here and line 64. I'm thinking of something like:

ui::Accelerator GetCloseAccelerator() {
  return ui::Accelerator...;
}

In an anonmyous namespace.

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:97: // Returns the created window.
Document the lifetime of the returned window and who owns it.

https://codereview.chromium.org/2871073002/diff/60001/ui/web_dialogs/web_dial...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/2871073002/diff/60001/ui/web_dialogs/web_dial...
ui/web_dialogs/web_dialog_delegate.h:141: // Returns the accelerators that can
be processed to this dialog.
Returns the accelerators handled by the delegate.

https://codereview.chromium.org/2871073002/diff/60001/ui/web_dialogs/web_dial...
ui/web_dialogs/web_dialog_delegate.h:144: // Returns true if |accelerator|
pressed is processed, otherwise false.
remove 'pressed'.

[Qiang(Joe) Xu, 2017-05-12 17:55:21]

Patchset #5 (id:80001) has been deleted

[Qiang(Joe) Xu, 2017-05-12 21:50:00]

Description was changed from

==========
cros: Bind hangup red button on remote controller to close web dialog

Changes:
The hangup red button on remote controller is mapped to 'Shift + Browser Back
(F1)'. Add this accelerator to allow closing web dialog. 

BUG=700128
TEST=tested with hotrod remote controller
==========

to

==========
cros: Bind hangup red button on remote controller to close web dialog

Changes:
The hangup red button on remote controller is mapped to 'Shift + Browser Back
(F1)'. Add this accelerator to allow closing web dialog. 

BUG=700128
TEST=tested with hotrod remote controller
and added test coverage
==========

[Qiang(Joe) Xu, 2017-05-12 21:50:45]

Hi all, comments addressed and test coverage added, PTAL, thanks

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_web_dialog.cc (right):

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:64:
ui::Accelerator(ui::VKEY_BROWSER_BACK, ui::EF_SHIFT_DOWN));
On 2017/05/11 04:48:27, xiyuan wrote:
> Move this into GetAccelerators and get rid of |accelerators_| member.

Done.

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:74: dialog_window_ =
chrome::ShowWebDialogInContainer(
On 2017/05/11 15:04:11, sky wrote:
> Initialize dialog_window_ to null.

Done.

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:185: if
(accelerator.key_code() == ui::VKEY_BROWSER_BACK &&
On 2017/05/11 15:04:11, sky wrote:
> Have a function that you use to get the accelerator so that you don't need to
> duplicate it here and line 64. I'm thinking of something like:
> 
> ui::Accelerator GetCloseAccelerator() {
>   return ui::Accelerator...;
> }
> 
> In an anonmyous namespace.

Done.

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_dialogs.h (right):

https://codereview.chromium.org/2871073002/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_dialogs.h:97: // Returns the created window.
On 2017/05/11 15:04:11, sky wrote:
> Document the lifetime of the returned window and who owns it.

Add the same comment as above: The window is automatically destroyed when it is
closed.

https://codereview.chromium.org/2871073002/diff/60001/ui/web_dialogs/web_dial...
File ui/web_dialogs/web_dialog_delegate.h (right):

https://codereview.chromium.org/2871073002/diff/60001/ui/web_dialogs/web_dial...
ui/web_dialogs/web_dialog_delegate.h:141: // Returns the accelerators that can
be processed to this dialog.
On 2017/05/11 15:04:11, sky wrote:
> Returns the accelerators handled by the delegate.

Done.

https://codereview.chromium.org/2871073002/diff/60001/ui/web_dialogs/web_dial...
ui/web_dialogs/web_dialog_delegate.h:144: // Returns true if |accelerator|
pressed is processed, otherwise false.
On 2017/05/11 15:04:11, sky wrote:
> remove 'pressed'.

Done.

[Qiang(Joe) Xu, 2017-05-12 21:52:08]

The CQ bit was checked by warx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 21:53:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2017-05-12 22:29:57]

lgtm

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_web_dialog.cc (right):

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:182: return accelerators;
nit: return {GetCloseAccelerator()};

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_web_dialog_browsertest.cc (right):

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_web_dialog_browsertest.cc:37:
aura::Window* window = dialog->get_dialog_window_for_test();
nit: aura::Window* -> gfx::NativeWindow

https://codereview.chromium.org/2871073002/diff/120001/ui/views/test/widget_t...
File ui/views/test/widget_test.cc (right):

https://codereview.chromium.org/2871073002/diff/120001/ui/views/test/widget_t...
ui/views/test/widget_test.cc:182: return;
DCHECK(widget) or just get rid of it to let it crash.

[Qiang(Joe) Xu, 2017-05-12 22:39:33]

Updated, sky@ PTAL

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_web_dialog.cc (right):

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_web_dialog.cc:182: return accelerators;
On 2017/05/12 22:29:57, xiyuan wrote:
> nit: return {GetCloseAccelerator()};

Done.

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/ui/login_web_dialog_browsertest.cc (right):

https://codereview.chromium.org/2871073002/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/login/ui/login_web_dialog_browsertest.cc:37:
aura::Window* window = dialog->get_dialog_window_for_test();
On 2017/05/12 22:29:57, xiyuan wrote:
> nit: aura::Window* -> gfx::NativeWindow

Done.

https://codereview.chromium.org/2871073002/diff/120001/ui/views/test/widget_t...
File ui/views/test/widget_test.cc (right):

https://codereview.chromium.org/2871073002/diff/120001/ui/views/test/widget_t...
ui/views/test/widget_test.cc:182: return;
On 2017/05/12 22:29:57, xiyuan wrote:
> DCHECK(widget) or just get rid of it to let it crash.

done by removing

[Qiang(Joe) Xu, 2017-05-12 22:39:59]

Patchset #7 (id:140001) has been deleted

[sky, 2017-05-14 16:05:12]

LGTM

[Qiang(Joe) Xu, 2017-05-15 17:06:07]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-05-15 17:06:08]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/2871073002/#ps160001
(title: "nits")

[commit-bot: I haz the power, 2017-05-15 17:06:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 17:55:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 17:55:06]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Qiang(Joe) Xu, 2017-05-15 17:58:10]

The CQ bit was checked by warx@chromium.org

[commit-bot: I haz the power, 2017-05-15 17:59:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 19:31:06]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494871090346030,
"parent_rev": "2dd54512772385e489fbcfedc2541cf719e7cda9", "commit_rev":
"9e608edc70f96a0d9ce6aef52d88928d851a7fe8"}

[commit-bot: I haz the power, 2017-05-15 19:31:15]

Description was changed from

==========
cros: Bind hangup red button on remote controller to close web dialog

Changes:
The hangup red button on remote controller is mapped to 'Shift + Browser Back
(F1)'. Add this accelerator to allow closing web dialog. 

BUG=700128
TEST=tested with hotrod remote controller
and added test coverage
==========

to

==========
cros: Bind hangup red button on remote controller to close web dialog

Changes:
The hangup red button on remote controller is mapped to 'Shift + Browser Back
(F1)'. Add this accelerator to allow closing web dialog. 

BUG=700128
TEST=tested with hotrod remote controller
and added test coverage

Review-Url: https://codereview.chromium.org/2871073002
Cr-Commit-Position: refs/heads/master@{#471864}
Committed:
https://chromium.googlesource.com/chromium/src/+/9e608edc70f96a0d9ce6aef52d88...
==========

[commit-bot: I haz the power, 2017-05-15 19:31:18]

Committed patchset #7 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/9e608edc70f96a0d9ce6aef52d88...