Make sure Search+<key(s)> are un-interceptable.

Make sure Search+<key(s)> are un-interceptable.

On ChromeOS, accelerators using the Search key as a modifier are considered
system accelerators, and they cannot be intercepted by apps or sites.

R=oshima@chromium.org
BUG=567294
Committed: https://crrev.com/c23ea2a280fd3bb8201ffacc72d5ac762c998553
Cr-Commit-Position: refs/heads/master@{#364533}

[afakhry, 2015-12-07 21:29:44]

Oshima, please take a look. Thanks!

[afakhry, 2015-12-08 18:22:57]

On 2015/12/07 21:29:44, afakhry wrote:
> Oshima, please take a look. Thanks!

Oshima?

[oshima, 2015-12-08 19:24:41]

can you add unit test?

https://codereview.chromium.org/1502993004/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_delegate.cc (right):

https://codereview.chromium.org/1502993004/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_delegate.cc:56: if (accelerator.IsCmdDown())
This prevent browser to process search+key. Can you double check if there no
browser shortcut key that we may want to change in near future?

[afakhry, 2015-12-09 00:50:23]

Added a unit test.

https://codereview.chromium.org/1502993004/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_delegate.cc (right):

https://codereview.chromium.org/1502993004/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_delegate.cc:56: if (accelerator.IsCmdDown())
On 2015/12/08 19:24:41, oshima wrote:
> This prevent browser to process search+key. Can you double check if there no
> browser shortcut key that we may want to change in near future?

c/b/ui/views/accelerator_table.cc doesn't have any shortcuts that uses COMMAND
as a modifier. Even the key VKEY_BROWSER_SEARCH is used only on non-ChromeOS
builds.

[oshima, 2015-12-09 20:12:45]

lgtm

https://codereview.chromium.org/1502993004/diff/1/ash/accelerators/accelerato...
File ash/accelerators/accelerator_delegate.cc (right):

https://codereview.chromium.org/1502993004/diff/1/ash/accelerators/accelerato...
ash/accelerators/accelerator_delegate.cc:56: if (accelerator.IsCmdDown())
On 2015/12/09 00:50:23, afakhry wrote:
> On 2015/12/08 19:24:41, oshima wrote:
> > This prevent browser to process search+key. Can you double check if there no
> > browser shortcut key that we may want to change in near future?
> 
> c/b/ui/views/accelerator_table.cc doesn't have any shortcuts that uses COMMAND
> as a modifier. Even the key VKEY_BROWSER_SEARCH is used only on non-ChromeOS
> builds.

I wanted to make sure that there is no future plan to change (like task manager)
in browser side, not now. If you're sure, i'm fine.

https://codereview.chromium.org/1502993004/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_filter_unittest.cc (right):

https://codereview.chromium.org/1502993004/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_filter_unittest.cc:154:
Shell::GetInstance()->DismissAppList();
I didn't fully understand what this test case does. Can you clarify?

[afakhry, 2015-12-10 00:18:40]

https://codereview.chromium.org/1502993004/diff/20001/ash/accelerators/accele...
File ash/accelerators/accelerator_filter_unittest.cc (right):

https://codereview.chromium.org/1502993004/diff/20001/ash/accelerators/accele...
ash/accelerators/accelerator_filter_unittest.cc:154:
Shell::GetInstance()->DismissAppList();
On 2015/12/09 20:12:44, oshima wrote:
> I didn't fully understand what this test case does. Can you clarify?

I remember that the app list is dismissed when an app is launched here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

, and that's why we have this check here:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/accelerators/a...

So this test simulates launching an app, and making sure it can't steal the
Search key.

[afakhry, 2015-12-10 23:01:36]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-12-10 23:03:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1502993004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1502993004/20001

[commit-bot: I haz the power, 2015-12-10 23:55:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-10 23:57:34]

Description was changed from

==========
Make sure Search+<key(s)> are un-interceptable.

On ChromeOS, accelerators using the Search key as a modifier are considered
system accelerators, and they cannot be intercepted by apps or sites.

R=oshima@chromium.org
BUG=567294
==========

to

==========
Make sure Search+<key(s)> are un-interceptable.

On ChromeOS, accelerators using the Search key as a modifier are considered
system accelerators, and they cannot be intercepted by apps or sites.

R=oshima@chromium.org
BUG=567294

Committed: https://crrev.com/c23ea2a280fd3bb8201ffacc72d5ac762c998553
Cr-Commit-Position: refs/heads/master@{#364533}
==========

[commit-bot: I haz the power, 2015-12-10 23:57:35]

Patchset 2 (id:??) landed as
https://crrev.com/c23ea2a280fd3bb8201ffacc72d5ac762c998553
Cr-Commit-Position: refs/heads/master@{#364533}