Regression: Search+Key pops up app launcher

Regression: Search+Key pops up app launcher

We decided to fix this issue even though it only happens when the
following 'wrong' key sequence is pressed:
- SearchKey [pressed]
- Key [pressed]
- SearchKey [released]
- Key [released]

Resulted wrong behavior: the AppLancher appears.

The Cause:
----------
When the search key is used as a modifier, the following pressed key is
rewritten and consumed. It doesn't get forwarded to
AcceleratorController managed by the AcceleratorManager in the
FocusManager. The AcceleratorController needs to know the previous
Accelerator to determine the correct behavior.

The 'Suggested' Fix:
--------------------
Track the system-wide current and previous key accelerators and require
that the ToggleAppList would only be triggered if LWIN [pressed] is
immediately followed by LWIN [released].

BUG=410835
TEST=manual, ash_unittests

Committed: https://crrev.com/5546373f24680054ad4e1eca425b5b1b7291dfe9
Cr-Commit-Position: refs/heads/master@{#305278}

[afakhry, 2014-11-13 22:20:23]

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

[Jun Mukai, 2014-11-14 02:09:47]

mukai@chromium.org changed reviewers:
+ mukai@chromium.org

[Jun Mukai, 2014-11-14 02:09:48]

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (left):

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:473: return false;
Please keep early-exit code if you don't have to.  Seems conditions are not
changed.  Early-exit reduces indent depth and makes code concise.

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:474: if (key_code == ui::VKEY_LWIN)
What is the motivation of removing this condition?

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:1118: const ui::Accelerator&
AcceleratorController::GetPreviousAccelerator() const {
Why do we keep two different previous accelerators?

https://codereview.chromium.org/727583002/diff/20001/ui/base/accelerators/acc...
File ui/base/accelerators/accelerator_history.cc (right):

https://codereview.chromium.org/727583002/diff/20001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.cc:39: if (event.type() !=
ui::ET_KEY_PRESSED && event.type() != ui::ET_KEY_RELEASED)
Can this line be IsKeyEvent() instead?

https://codereview.chromium.org/727583002/diff/20001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.cc:44: int modifiers =
CalculateModifiers(event);
why not directly using event.flags()?

https://codereview.chromium.org/727583002/diff/20001/ui/events/DEPS
File ui/events/DEPS (right):

https://codereview.chromium.org/727583002/diff/20001/ui/events/DEPS#newcode2
ui/events/DEPS:2: "+ui/base",
Aagh.  ui/base already depends on ui/events, so adding this will lead to a cycle
dependency, which doesn't look great.

Could you find the way to avoid this?
I think building accelerator can be done as a PreHandleEventFilter

[afakhry, 2014-11-14 17:30:58]

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (left):

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:474: if (key_code == ui::VKEY_LWIN)
On 2014/11/14 02:09:48, Jun Mukai wrote:
> What is the motivation of removing this condition?

Because in my code, I already made sure that key_code == ui::VKEY_LWIN. No need
to redo the check.

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/727583002/diff/20001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:1118: const ui::Accelerator&
AcceleratorController::GetPreviousAccelerator() const {
On 2014/11/14 02:09:48, Jun Mukai wrote:
> Why do we keep two different previous accelerators?

This is needed for some unit tests that directly calls
AcceleratorController::Process() without going through the events system.

https://codereview.chromium.org/727583002/diff/20001/ui/base/accelerators/acc...
File ui/base/accelerators/accelerator_history.cc (right):

https://codereview.chromium.org/727583002/diff/20001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.cc:39: if (event.type() !=
ui::ET_KEY_PRESSED && event.type() != ui::ET_KEY_RELEASED)
On 2014/11/14 02:09:48, Jun Mukai wrote:
> Can this line be IsKeyEvent() instead?

No, because we want to avoid types like ET_TRANSLATED_KEY_PRESS, and
ET_TRANSLATED_KEY_RELEASE.

https://codereview.chromium.org/727583002/diff/20001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.cc:44: int modifiers =
CalculateModifiers(event);
On 2014/11/14 02:09:48, Jun Mukai wrote:
> why not directly using event.flags()?

We are only interested in the flags used by the Accelerator. However, I think we
can do something more clever than if/elses.. Thanks for pointing this out!

[Jun Mukai, 2014-11-15 01:56:33]

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:473: accelerator.type() !=
ui::ET_KEY_RELEASED)
This was weird in the previous code.  Multi-lined if should have curly braces.
Could you add that?

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:473: accelerator.type() !=
ui::ET_KEY_RELEASED)
This means accelerator is not handled at all if key code is not VKEY_LWIN.  Is
that true?
accelerator_table has a mapping of VKEY_BROWSER_SEARCH.  Isn't it currently
used?

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.h:142: 
remove this blank line

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:250: static bool
ProcessInControllerForTest(const ui::Accelerator& accelerator) {
You don't have to say 'ForTest', the entire file is already for test :)

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:1052:
GetCurrentAccelerator().key_code());
Can you add
EXPECT_FALSE(ash::Shell::GetInstance()->GetAppListTargetVisibility()); here ?

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc#newcode932
ash/shell.cc:932: AddPreTargetHandler(ui::AcceleratorHistory::GetInstance());
Can we move it into accelerator_filter_ instead of here?

https://codereview.chromium.org/727583002/diff/40001/ui/base/accelerators/acc...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/40001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.h:41: // From ui::EventHandler
common style of the comment is:
// ui::EventHandler:

[afakhry, 2014-11-15 02:40:31]

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:473: accelerator.type() !=
ui::ET_KEY_RELEASED)
On 2014/11/15 01:56:33, Jun Mukai wrote:
> This means accelerator is not handled at all if key code is not VKEY_LWIN.  Is
> that true?
> accelerator_table has a mapping of VKEY_BROWSER_SEARCH.  Isn't it currently
> used?

I added a test for VKEY_BROWSER_SEARCH and pressed.

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:473: accelerator.type() !=
ui::ET_KEY_RELEASED)
On 2014/11/15 01:56:33, Jun Mukai wrote:
> This was weird in the previous code.  Multi-lined if should have curly braces.
> Could you add that?

Done.

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.h:142: 
On 2014/11/15 01:56:33, Jun Mukai wrote:
> remove this blank line

Done.

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:250: static bool
ProcessInControllerForTest(const ui::Accelerator& accelerator) {
On 2014/11/15 01:56:33, Jun Mukai wrote:
> You don't have to say 'ForTest', the entire file is already for test :)

Done.

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller_unittest.cc:1052:
GetCurrentAccelerator().key_code());
On 2014/11/15 01:56:33, Jun Mukai wrote:
> Can you add
> EXPECT_FALSE(ash::Shell::GetInstance()->GetAppListTargetVisibility()); here ?

Done.

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc#newcode932
ash/shell.cc:932: AddPreTargetHandler(ui::AcceleratorHistory::GetInstance());
On 2014/11/15 01:56:33, Jun Mukai wrote:
> Can we move it into accelerator_filter_ instead of here?

I already tried that, but it didn't work. It was causing a failure in the unit
tests. Accelerator filter has different conditions for creating and accelerator
than those needed by the focus manager and the accelerator controller.

I can add a TODO(afakhry) to figure out a way to combine them.

https://codereview.chromium.org/727583002/diff/40001/ui/base/accelerators/acc...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/40001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.h:41: // From ui::EventHandler
On 2014/11/15 01:56:33, Jun Mukai wrote:
> common style of the comment is:
> // ui::EventHandler:

Done.

[afakhry, 2014-11-15 03:04:58]

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
File ash/accelerators/accelerator_controller.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/accelerators/acceler...
ash/accelerators/accelerator_controller.cc:481: DCHECK_EQ(ui::VKEY_LWIN,
accelerator.key_code());
I also removed this line as it doesn't take into consideration
VKEY_BROWSER_SEARCH

[Jun Mukai, 2014-11-17 17:39:20]

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc#newcode932
ash/shell.cc:932: AddPreTargetHandler(ui::AcceleratorHistory::GetInstance());
On 2014/11/15 02:40:31, afakhry wrote:
> On 2014/11/15 01:56:33, Jun Mukai wrote:
> > Can we move it into accelerator_filter_ instead of here?
> 
> I already tried that, but it didn't work. It was causing a failure in the unit
> tests. Accelerator filter has different conditions for creating and
accelerator
> than those needed by the focus manager and the accelerator controller.
> 
> I can add a TODO(afakhry) to figure out a way to combine them.

Which test fails for why?  It does not make much sense doing like this for test.
How hard to fix the test code?

https://codereview.chromium.org/727583002/diff/60001/ui/base/accelerators/acc...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/60001/ui/base/accelerators/acc...
ui/base/accelerators/accelerator_history.h:41: // ui::EventHandler
colon at the end of the line

https://codereview.chromium.org/727583002/diff/60001/ui/views/focus/focus_man...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/727583002/diff/60001/ui/views/focus/focus_man...
ui/views/focus/focus_manager.cc:64: GetCurrentAccelerator());
Can we revert this?   Is there any specific reason that focus_manager have to
use the "current" accelerator?

[afakhry, 2014-11-17 23:10:18]

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc
File ash/shell.cc (right):

https://codereview.chromium.org/727583002/diff/40001/ash/shell.cc#newcode932
ash/shell.cc:932: AddPreTargetHandler(ui::AcceleratorHistory::GetInstance());
On 2014/11/17 17:39:20, Jun Mukai wrote:
> On 2014/11/15 02:40:31, afakhry wrote:
> > On 2014/11/15 01:56:33, Jun Mukai wrote:
> > > Can we move it into accelerator_filter_ instead of here?
> > 
> > I already tried that, but it didn't work. It was causing a failure in the
unit
> > tests. Accelerator filter has different conditions for creating and
> accelerator
> > than those needed by the focus manager and the accelerator controller.
> > 
> > I can add a TODO(afakhry) to figure out a way to combine them.
> 
> Which test fails for why?  It does not make much sense doing like this for
test.
> How hard to fix the test code?

Done.

https://codereview.chromium.org/727583002/diff/60001/ui/views/focus/focus_man...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/727583002/diff/60001/ui/views/focus/focus_man...
ui/views/focus/focus_manager.cc:64: GetCurrentAccelerator());
On 2014/11/17 17:39:20, Jun Mukai wrote:
> Can we revert this?   Is there any specific reason that focus_manager have to
> use the "current" accelerator?

Done.

[Jun Mukai, 2014-11-18 01:44:03]

can we stop doing singleton and share the instance between
accelerator_controller and accelerator_filter?

Both are initialized in ash/shell, therefore this can be achieved by:
- let accelerator_controller owns accelerator_history, allow accessing it
- pass the owned accelerator_history as a constructor parameter to
accelerator_filter

thoughts?

[afakhry, 2014-11-18 02:18:01]

On 2014/11/18 01:44:03, Jun Mukai wrote:
> can we stop doing singleton and share the instance between
> accelerator_controller and accelerator_filter?
> 
> Both are initialized in ash/shell, therefore this can be achieved by:
> - let accelerator_controller owns accelerator_history, allow accessing it
> - pass the owned accelerator_history as a constructor parameter to
> accelerator_filter
> 
> thoughts?

Agreed, since Ash Shell manages the only instance of AcceleratorController.

[Jun Mukai, 2014-11-18 17:33:44]

Also edit athena files which use accelerator_filter.  Although we won't use
athena files anymore, we still want not to fail to build, and not to crash when
running for the time being.

https://codereview.chromium.org/727583002/diff/100001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/727583002/diff/100001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:148: ui::AcceleratorHistory*
accelerator_history_;
It's better to be owned by this class (or let accelerator_filter own it). Not by
shell.

https://codereview.chromium.org/727583002/diff/100001/ui/base/accelerators/ac...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/100001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:19: //
----------------------------------------------------------------------
Usually comment for class description doesn't have decoration like ------.

[afakhry, 2014-11-18 19:23:52]

https://codereview.chromium.org/727583002/diff/100001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/727583002/diff/100001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:148: ui::AcceleratorHistory*
accelerator_history_;
On 2014/11/18 17:33:44, Jun Mukai wrote:
> It's better to be owned by this class (or let accelerator_filter own it). Not
by
> shell.

Done.

https://codereview.chromium.org/727583002/diff/100001/ui/base/accelerators/ac...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/100001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:19: //
----------------------------------------------------------------------
On 2014/11/18 17:33:44, Jun Mukai wrote:
> Usually comment for class description doesn't have decoration like ------.

Done.

[Jun Mukai, 2014-11-18 21:46:21]

lgtm

however -- I'm not the owner of those files. Please wait for the review of the
owners.

[afakhry, 2014-11-18 21:51:48]

On 2014/11/18 21:46:21, Jun Mukai wrote:
> lgtm
> 
> however -- I'm not the owner of those files. Please wait for the review of the
> owners.

@sky
Could you please review this?

[sky, 2014-11-18 22:11:48]

LGTM with the following fixed

https://codereview.chromium.org/727583002/diff/120001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/727583002/diff/120001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:256: static ui::Accelerator
GetPreviousAccelerator() {
const ui::Accelerator& here and next one.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
File ui/base/accelerators/accelerator_history.cc (right):

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.cc:38: }; // namespace ui
no ;

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.cc:39: 
remove empty newlines.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:28: const Accelerator&
GetPreviousAccelerator() const;
inline these and name to match the fields, eg previous_accelerator()

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:35: 
nit: no newline here.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:38: };
DISALLOW...

https://codereview.chromium.org/727583002/diff/120001/ui/wm/core/accelerator_...
File ui/wm/core/accelerator_filter.h (right):

https://codereview.chromium.org/727583002/diff/120001/ui/wm/core/accelerator_...
ui/wm/core/accelerator_filter.h:26: ui::AcceleratorHistory*
accelerator_history);
Document ownership here.

[afakhry, 2014-11-18 23:08:40]

https://codereview.chromium.org/727583002/diff/120001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller_unittest.cc (right):

https://codereview.chromium.org/727583002/diff/120001/ash/accelerators/accele...
ash/accelerators/accelerator_controller_unittest.cc:256: static ui::Accelerator
GetPreviousAccelerator() {
On 2014/11/18 22:11:48, sky wrote:
> const ui::Accelerator& here and next one.

Done.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
File ui/base/accelerators/accelerator_history.cc (right):

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.cc:38: }; // namespace ui
On 2014/11/18 22:11:48, sky wrote:
> no ;

Done.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.cc:39: 
On 2014/11/18 22:11:48, sky wrote:
> remove empty newlines.

Done.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
File ui/base/accelerators/accelerator_history.h (right):

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:28: const Accelerator&
GetPreviousAccelerator() const;
On 2014/11/18 22:11:48, sky wrote:
> inline these and name to match the fields, eg previous_accelerator()

Done.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:35: 
On 2014/11/18 22:11:48, sky wrote:
> nit: no newline here.

Done.

https://codereview.chromium.org/727583002/diff/120001/ui/base/accelerators/ac...
ui/base/accelerators/accelerator_history.h:38: };
On 2014/11/18 22:11:48, sky wrote:
> DISALLOW...

Done.

https://codereview.chromium.org/727583002/diff/120001/ui/wm/core/accelerator_...
File ui/wm/core/accelerator_filter.h (right):

https://codereview.chromium.org/727583002/diff/120001/ui/wm/core/accelerator_...
ui/wm/core/accelerator_filter.h:26: ui::AcceleratorHistory*
accelerator_history);
On 2014/11/18 22:11:48, sky wrote:
> Document ownership here.

Done.

[afakhry, 2014-11-18 23:25:12]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2014-11-18 23:26:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727583002/140001

[commit-bot: I haz the power, 2014-11-18 23:44:41]

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

[commit-bot: I haz the power, 2014-11-18 23:44:42]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[afakhry, 2014-11-19 01:42:37]

Added one line at the end of files for Mac.
Modified Build.gn in /ui/base/
Guarded the test for ui::VKEY_BROWSER_SEARCH with #if defined(OS_CHROMEOS)

[afakhry, 2014-11-19 01:42:56]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2014-11-19 01:44:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727583002/160001

[commit-bot: I haz the power, 2014-11-19 02:38:17]

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

[commit-bot: I haz the power, 2014-11-19 02:38:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[afakhry, 2014-11-20 02:53:01]

On 2014/11/19 02:38:19, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

The StickyKeysBrowserTest.SearchLeftOmnibox used to assume that the selection
bounds of the omnibox was set to the beginning due to the LWIN+LEFT event, while
in fact it was reset to the beginning as the omnibox loses focus (OnBlur) since
the AppList was erronously displayed.

[Jun Mukai, 2014-11-20 03:05:46]

lgtm++
But please receive review from dmazzoni (or someone owning
chrome/browser/chromeos/accessibility)

[afakhry, 2014-11-20 03:08:05]

afakhry@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[afakhry, 2014-11-20 19:55:09]

On 2014/11/20 03:08:06, afakhry wrote:

@dmazzoni
Could you please take a look at the change introduced to
'chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc'?

[dmazzoni, 2014-11-20 21:32:38]

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc (right):

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc:46: void
SendKeyPress(ui::KeyboardCode key) {
Your changes are fine, but I don't like that some tests use
ui_test_utils::SendKeyPress... and others use EventGenerator. If EventGenerator
is better, can we just always use that?

Can you just delete all of the SendKeyPress functions and can we call
EventGenerator functions instead, everywhere here?

Alternatively, could you leave the tests alone, and instead rewrite the
SendKeyPress helper functions using EventGenerator?

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc:58:
ui::test::EventGenerator& GetEventGenerator() {
Please just move this to SetUp

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc:174:
generator.PressKey(ui::VKEY_F, ui::EF_NONE);
Just call generator_.PressKey, then there's no need to call GetEventGenerator
from each test.

[afakhry, 2014-11-21 02:15:03]

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc (right):

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc:46: void
SendKeyPress(ui::KeyboardCode key) {
On 2014/11/20 21:32:38, dmazzoni wrote:
> Your changes are fine, but I don't like that some tests use
> ui_test_utils::SendKeyPress... and others use EventGenerator. If
EventGenerator
> is better, can we just always use that?
> 
> Can you just delete all of the SendKeyPress functions and can we call
> EventGenerator functions instead, everywhere here?
> 
> Alternatively, could you leave the tests alone, and instead rewrite the
> SendKeyPress helper functions using EventGenerator?

Done.

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc:58:
ui::test::EventGenerator& GetEventGenerator() {
On 2014/11/20 21:32:38, dmazzoni wrote:
> Please just move this to SetUp
> 

Moved to SetUpOnMainThread() since SetUp() caused it to crash by not having
event_generator_ initialized in time.

https://codereview.chromium.org/727583002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/sticky_keys_browsertest.cc:58:
ui::test::EventGenerator& GetEventGenerator() {
On 2014/11/20 21:32:38, dmazzoni wrote:
> Please just move this to SetUp
> 

Done.

[dmazzoni, 2014-11-21 06:25:28]

lgtm

Thanks!

[afakhry, 2014-11-21 17:24:12]

On 2014/11/21 06:25:28, dmazzoni wrote:
> lgtm
> 
> Thanks!

You're welcome! :)

[afakhry, 2014-11-21 17:24:21]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2014-11-21 17:24:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727583002/200001

[commit-bot: I haz the power, 2014-11-21 17:28:07]

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

[commit-bot: I haz the power, 2014-11-21 17:28:08]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/84969)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[afakhry, 2014-11-21 18:43:37]

On 2014/11/21 17:28:08, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_gpu on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/84969)
>   android_arm64_dbg_recipe on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
>   android_chromium_gn_compile_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
>   linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   linux_chromium_gn_rel on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   win8_chromium_rel on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
>   win_chromium_compile_dbg on tryserver.chromium.win
>
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

Fixed the stale patch

[afakhry, 2014-11-21 18:44:10]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2014-11-21 18:44:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727583002/220001

[commit-bot: I haz the power, 2014-11-21 18:49:58]

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

[commit-bot: I haz the power, 2014-11-21 18:50:00]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/85020)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[afakhry, 2014-11-21 20:07:24]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2014-11-21 20:08:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727583002/240001

[commit-bot: I haz the power, 2014-11-21 21:06:29]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-11-21 21:07:13]

Patchset 13 (id:??) landed as
https://crrev.com/5546373f24680054ad4e1eca425b5b1b7291dfe9
Cr-Commit-Position: refs/heads/master@{#305278}