Do not activate system tray bubble by default

Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated. I'll revisit this after this CL as going through all notification is a huge task.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase coverage.

Committed: https://crrev.com/d69e8cbbe0412fabce0c091e6d19d9ac9d1fea00
Cr-Commit-Position: refs/heads/master@{#420794}

[oshima, 2016-09-14 02:42:43]

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

[commit-bot: I haz the power, 2016-09-14 02:43:00]

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, 2016-09-14 02:52:34]

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

[commit-bot: I haz the power, 2016-09-14 02:52:35]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[oshima, 2016-09-14 16:38:39]

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

[commit-bot: I haz the power, 2016-09-14 16:38:55]

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, 2016-09-14 16:42:26]

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

[commit-bot: I haz the power, 2016-09-14 16:42:27]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[oshima, 2016-09-14 16:52:01]

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

[commit-bot: I haz the power, 2016-09-14 16:52:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-09-14 16:52:57]

Description was changed from

==========
Close

BUG=
==========

to

==========
Do not activate system tray bubble

BUG=120680
==========

[oshima, 2016-09-14 16:53:07]

Patchset #2 (id:20001) has been deleted

[oshima, 2016-09-14 16:53:12]

Patchset #2 (id:40001) has been deleted

[oshima, 2016-09-14 16:53:17]

Patchset #2 (id:60001) has been deleted

[oshima, 2016-09-14 16:53:23]

Patchset #1 (id:1) has been deleted

[oshima, 2016-09-14 16:53:32]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2016-09-14 17:56:51]

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

[commit-bot: I haz the power, 2016-09-14 17:56:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[oshima, 2016-09-21 15:48:57]

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

[commit-bot: I haz the power, 2016-09-21 15:49:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-09-21 16:01:13]

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

[commit-bot: I haz the power, 2016-09-21 16:01:49]

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, 2016-09-21 16:58:37]

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

[commit-bot: I haz the power, 2016-09-21 16:58:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2016-09-21 18:26:02]

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

[commit-bot: I haz the power, 2016-09-21 18:26:25]

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, 2016-09-21 19:18:07]

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

[commit-bot: I haz the power, 2016-09-21 19:18:08]

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

[oshima, 2016-09-21 19:20:54]

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

[commit-bot: I haz the power, 2016-09-21 19:21:13]

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, 2016-09-21 20:07:55]

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

[commit-bot: I haz the power, 2016-09-21 20:07:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2016-09-22 17:28:21]

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

[commit-bot: I haz the power, 2016-09-22 17:28:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-09-22 18:05:53]

Patchset #1 (id:100001) has been deleted

[oshima, 2016-09-22 18:06:16]

Patchset #1 (id:120001) has been deleted

[oshima, 2016-09-22 18:06:26]

Patchset #4 (id:200001) has been deleted

[oshima, 2016-09-22 18:06:30]

Patchset #3 (id:180001) has been deleted

[oshima, 2016-09-22 18:06:34]

Patchset #2 (id:160001) has been deleted

[oshima, 2016-09-22 18:06:39]

Patchset #1 (id:140001) has been deleted

[oshima, 2016-09-22 18:10:17]

Description was changed from

==========
Do not activate system tray bubble

BUG=120680
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in in active state by default,
  and explicitly close the bubble after executing action.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
==========

[oshima, 2016-09-22 18:11:38]

oshima@chromium.org changed reviewers:
+ dmazzoni@chromium.org, jamescook@chromium.org, stevenjb@chromium.org

[oshima, 2016-09-22 18:34:28]

jamescook@ -> ash
stevenjb@ -> ui/views/bubble
dmazzoni@ -> for accessibility

[oshima, 2016-09-22 18:35:14]

I'd like to land this this week so PTAL.

[stevenjb, 2016-09-22 19:21:20]

ui/views/bubble lgtm

[James Cook, 2016-09-22 21:17:17]

I'm a little worried that you might have missed a case where the system tray
bubble needs to be explicitly closed via CloseSystemBubble(). I don't know how
to find those cases, however, other than to push to dev channel and see how it
goes.

In the CL description, "Open SystemTrayBubble in in active state by default" ->
should that be "inactive"?

Also, please add a TEST= line to the CL description.

https://codereview.chromium.org/2330403002/diff/240001/ash/aura/wm_shell_aura.h
File ash/aura/wm_shell_aura.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/aura/wm_shell_aura...
ash/aura/wm_shell_aura.h:75: 
nit: no blank line

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
File ash/common/key_event_watcher.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
ash/common/key_event_watcher.cc:10: KeyEventWatcher::~KeyEventWatcher() {}
nit: blank line above

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
File ash/common/key_event_watcher.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
ash/common/key_event_watcher.h:23: class ASH_EXPORT KeyEventWatcher {
I like how this only listens for the keys it cares about, rather than all keys.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
ash/common/key_event_watcher.h:31: // pressed.
Document that the key event will be consumed if a matching accelerator is found.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
File ash/common/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
ash/common/system/chromeos/settings/tray_settings.cc:37: explicit
SettingsDefaultView(SystemTrayItem* owner, LoginStatus status)
nit: no explicit

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
ash/common/system/chromeos/settings/tray_settings.cc:40: label_(NULL),
nit: nullptr if you're touching the line

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
File ash/common/system/chromeos/tray_tracing.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
ash/common/system/chromeos/tray_tracing.cc:31:
DefaultTracingView(SystemTrayItem* owner) : ActionableView(owner) {
explicit

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/actionable_view.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/actionable_view.h:25: explicit
ActionableView(SystemTrayItem* owner);
nit: document that |owner| can be null, and what that means.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/actionable_view.h:32: void CloseSystemBubble();
nit: document

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:69: #include "base/debug/stack_trace.h"
remove

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:167: key_event_watcher_.reset();
Does this need to be both here and in destructor?

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:462: bool can_activate,
This needs to be renamed. It no longer means "can activate".

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:539: key_event_watcher_.reset();
optional: This might be clearer in an else{} clause below.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:540: if (can_activate) {
Document what you are doing in this block of code and/or split into a separate
function, since ShowItems() is > 100 lines long now.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:543: ui::Accelerator(ui::VKEY_ESCAPE, 0),
0 -> EF_NONE

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:171: // Activate the system tay bubble.
nit: tay -> tray

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:175: void CloseBubble(const ui::KeyEvent&
key_event);
nit: document. For example, why is it different than CloseSystemBubble.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:177: // Acitavates the bubble and starts
key navigation with the |key_event|.
nit: activates

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:206: TEST_F(SystemTrayTest,
Activation) {
Document what this is testing and reference the bug.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:489:
ASSERT_TRUE(tray->HasSystemBubble());
These should all be EXPECT_TRUE (the old test code is wrong).

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:494: {
no need for block scope here (or elsewhere in this test)

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (left):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:57: // a popup notification from
message center.
What happens if the system tray menu is open and you click inside of a popup
notification (like the screenshot notification)? The system tray menu should not
close -- does it still work that way?

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/upda...
File ash/common/system/update/tray_update.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/upda...
ash/common/system/update/tray_update.cc:83: explicit UpdateView(SystemTrayItem*
owner, const UpdateInfo& info)
no explicit

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:369: return std::unique_ptr<KeyEventWatcher>();
Please file a bug to implement this for mus, reference the bug here, and add a
NOTIMPLEMENTED(), like the other Create() functions above.

https://codereview.chromium.org/2330403002/diff/240001/ash/system/web_notific...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/system/web_notific...
ash/system/web_notification/web_notification_tray_unittest.cc:378:
UpdateAutoHideStateNow();
Would it work to call shelf->UpdateAutoHideState();
RunAllPendingInMessageLoop(); like below?

[oshima, 2016-09-23 09:26:25]

Description was changed from

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in in active state by default,
  and explicitly close the bubble after executing action.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
==========

[oshima, 2016-09-23 09:27:11]

Description was changed from

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
==========

[oshima, 2016-09-23 09:37:20]

https://codereview.chromium.org/2330403002/diff/240001/ash/aura/wm_shell_aura.h
File ash/aura/wm_shell_aura.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/aura/wm_shell_aura...
ash/aura/wm_shell_aura.h:75: 
On 2016/09/22 21:17:16, James Cook wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
File ash/common/key_event_watcher.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
ash/common/key_event_watcher.cc:10: KeyEventWatcher::~KeyEventWatcher() {}
On 2016/09/22 21:17:16, James Cook wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
File ash/common/key_event_watcher.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/key_event_w...
ash/common/key_event_watcher.h:31: // pressed.
On 2016/09/22 21:17:16, James Cook wrote:
> Document that the key event will be consumed if a matching accelerator is
found.

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
File ash/common/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
ash/common/system/chromeos/settings/tray_settings.cc:37: explicit
SettingsDefaultView(SystemTrayItem* owner, LoginStatus status)
On 2016/09/22 21:17:16, James Cook wrote:
> nit: no explicit

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
ash/common/system/chromeos/settings/tray_settings.cc:40: label_(NULL),
On 2016/09/22 21:17:16, James Cook wrote:
> nit: nullptr if you're touching the line

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
File ash/common/system/chromeos/tray_tracing.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/chro...
ash/common/system/chromeos/tray_tracing.cc:31:
DefaultTracingView(SystemTrayItem* owner) : ActionableView(owner) {
On 2016/09/22 21:17:16, James Cook wrote:
> explicit

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/actionable_view.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/actionable_view.h:25: explicit
ActionableView(SystemTrayItem* owner);
On 2016/09/22 21:17:16, James Cook wrote:
> nit: document that |owner| can be null, and what that means.

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/actionable_view.h:32: void CloseSystemBubble();
On 2016/09/22 21:17:16, James Cook wrote:
> nit: document

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:69: #include "base/debug/stack_trace.h"
On 2016/09/22 21:17:16, James Cook wrote:
> remove

Oops, done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:167: key_event_watcher_.reset();
On 2016/09/22 21:17:16, James Cook wrote:
> Does this need to be both here and in destructor?

Yes, it is in dtor too.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:462: bool can_activate,
On 2016/09/22 21:17:16, James Cook wrote:
> This needs to be renamed. It no longer means "can activate".

The name should have been "activate", and that's what is used in the header. I
think can_activate is actually better now because it can be activated by several
means. I updated the header instead.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:539: key_event_watcher_.reset();
On 2016/09/22 21:17:16, James Cook wrote:
> optional: This might be clearer in an else{} clause below.

I prefer this way because this can avoid potential issue in having two
KeyEventWatcher instances at the same time. (New one is created before
resetting)

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:540: if (can_activate) {
On 2016/09/22 21:17:16, James Cook wrote:
> Document what you are doing in this block of code and/or split into a separate
> function, since ShowItems() is > 100 lines long now.

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:543: ui::Accelerator(ui::VKEY_ESCAPE, 0),
On 2016/09/22 21:17:16, James Cook wrote:
> 0 -> EF_NONE

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray.h (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:171: // Activate the system tay bubble.
On 2016/09/22 21:17:16, James Cook wrote:
> nit: tay -> tray

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:175: void CloseBubble(const ui::KeyEvent&
key_event);
On 2016/09/22 21:17:16, James Cook wrote:
> nit: document. For example, why is it different than CloseSystemBubble.

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:177: // Acitavates the bubble and starts
key navigation with the |key_event|.
On 2016/09/22 21:17:16, James Cook wrote:
> nit: activates

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:206: TEST_F(SystemTrayTest,
Activation) {
On 2016/09/22 21:17:16, James Cook wrote:
> Document what this is testing and reference the bug.

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:489:
ASSERT_TRUE(tray->HasSystemBubble());
On 2016/09/22 21:17:17, James Cook wrote:
> These should all be EXPECT_TRUE (the old test code is wrong).

Yes, I noticed that. I tried to change them all, but decided not to change in
this CL but mistakely revered them as well. Fixed.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:494: {
On 2016/09/22 21:17:17, James Cook wrote:
> no need for block scope here (or elsewhere in this test)

Let me fix elsewhere in another CL.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (left):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:57: // a popup notification from
message center.
On 2016/09/22 21:17:17, James Cook wrote:
> What happens if the system tray menu is open and you click inside of a popup
> notification (like the screenshot notification)? The system tray menu should
not
> close -- does it still work that way?

Thanks for the catch. Looks like there is no good way to tell if it's a popup
notification,
so I added condition that is narrower.

I also added SystemTray::ActivationObserver because certain notification relies
on activation
to close the bubble (although I'm not sure these behaviors are by design, or
just happen to be so. wifi notification, for example, do nothing when clicked).
I still need the CloseSystemBubble change because these command does not
necessarily change activation.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/upda...
File ash/common/system/update/tray_update.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/upda...
ash/common/system/update/tray_update.cc:83: explicit UpdateView(SystemTrayItem*
owner, const UpdateInfo& info)
On 2016/09/22 21:17:17, James Cook wrote:
> no explicit

Done.

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:369: return std::unique_ptr<KeyEventWatcher>();
On 2016/09/22 21:17:17, James Cook wrote:
> Please file a bug to implement this for mus, reference the bug here, and add a
> NOTIMPLEMENTED(), like the other Create() functions above.

Done.

FYI: ImmersiveFullscreenController doesn't have it.

https://codereview.chromium.org/2330403002/diff/240001/ash/system/web_notific...
File ash/system/web_notification/web_notification_tray_unittest.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/system/web_notific...
ash/system/web_notification/web_notification_tray_unittest.cc:378:
UpdateAutoHideStateNow();
On 2016/09/22 21:17:17, James Cook wrote:
> Would it work to call shelf->UpdateAutoHideState();
> RunAllPendingInMessageLoop(); like below?

This is to deal with timer delay, not posted events, so this has to be this way.

The reason it was working before is that ShowDefaultView ends up calling
UpdateAutoHideState multiple times, which resets timer (because the state is the
same), which happens to update the state.

It now calls only once, so it will not stop the timer.

[oshima, 2016-09-23 09:38:04]

Description was changed from

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.
==========

[oshima, 2016-09-23 09:39:46]

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

[commit-bot: I haz the power, 2016-09-23 09:40:02]

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, 2016-09-23 10:18:42]

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

[commit-bot: I haz the power, 2016-09-23 10:18:44]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmazzoni, 2016-09-23 15:26:30]

lgtm

No concerns about the code.

You mention that it activates the system tray bubble if you
open it with the accelerator, I'm assuming you mean
Alt+Shift+S.

There's another way to activate it: Press Ctrl+F1 until the
system tray in the lower-right corner is focused, then press
Enter to open it. Does this activate it as well, or
essentially does keyboard access work well?

No concerns about landing it first. Let's test it when it
hits dev channel and watch out for regressions.

[oshima, 2016-09-23 16:08:56]

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

[commit-bot: I haz the power, 2016-09-23 16:09:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[oshima, 2016-09-23 16:09:57]

Description was changed from

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated. I'll revisit this after this CL as going through all notification
is a huge task.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.
==========

[James Cook, 2016-09-23 16:32:28]

Just little things. Feel free to ping me on chat when you want me to take a
look, I know you're trying to get this in today.

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:167: key_event_watcher_.reset();
On 2016/09/23 09:37:19, oshima (OOO till Sep. 27) wrote:
> On 2016/09/22 21:17:16, James Cook wrote:
> > Does this need to be both here and in destructor?
> 
> Yes, it is in dtor too.

I meant, does it have to be in the destructor? In theory all users of SystemTray
should be calling Shutdown() before they delete the object. If someone isn't
calling Shutdown() that sounds like a bug.

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:369: return std::unique_ptr<KeyEventWatcher>();
On 2016/09/23 09:37:20, oshima (OOO till Sep. 27) wrote:
> On 2016/09/22 21:17:17, James Cook wrote:
> > Please file a bug to implement this for mus, reference the bug here, and add
a
> > NOTIMPLEMENTED(), like the other Create() functions above.
> 
> Done.
> 
> FYI: ImmersiveFullscreenController doesn't have it.

ImmersiveFullscreenController lives in //ash/shared, which means it has been
ported to mustash.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/chro...
File ash/common/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/chro...
ash/common/system/chromeos/settings/tray_settings.cc:146: return nullptr;
FYI, this is fine, but if you're worried about patch failures while backporting
I'm also OK with leaving these as NULL. Generally I only convert NULL -> nullptr
if I'm editing the line for another reason (like clang-format rewrapping).

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:131:
WmShell::Get()->AddActivationObserver(this);
optional: DCHECK(tray_).

(I've started using DCHECK for preconditions for functions, but it's just a
style thing.)

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/system_tray.h (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:183: // Creates the key event watcher.
nit: "See ShowItems() for why key events are observed."

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:214: // * When the tray item is set to be
focused.
Nice documentation.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/system_tray_unittest.cc:231:
EXPECT_FALSE(widget->IsActive());
Thanks for adding more tests.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:59: window->GetType() ==
ui::wm::WINDOW_TYPE_POPUP &&
Any particular reason you're limiting this to always-on-top popup windows? Is it
because of clicking on the status area widget?

https://codereview.chromium.org/2330403002/diff/280001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:369: // crbug.com/
Bug number please.

[commit-bot: I haz the power, 2016-09-23 17:02:37]

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

[commit-bot: I haz the power, 2016-09-23 17:02:38]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-09-23 19:19:54]

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:167: key_event_watcher_.reset();
On 2016/09/23 16:32:28, James Cook wrote:
> On 2016/09/23 09:37:19, oshima (OOO till Sep. 27) wrote:
> > On 2016/09/22 21:17:16, James Cook wrote:
> > > Does this need to be both here and in destructor?
> > 
> > Yes, it is in dtor too.
> 
> I meant, does it have to be in the destructor? In theory all users of
SystemTray
> should be calling Shutdown() before they delete the object. If someone isn't
> calling Shutdown() that sounds like a bug.

It should actually be dtor, where the bubble is reset. This Shutdown is only to
reset the web_notification_tray_, so i removed them.

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2330403002/diff/240001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:369: return std::unique_ptr<KeyEventWatcher>();
On 2016/09/23 16:32:28, James Cook wrote:
> On 2016/09/23 09:37:20, oshima (OOO till Sep. 27) wrote:
> > On 2016/09/22 21:17:17, James Cook wrote:
> > > Please file a bug to implement this for mus, reference the bug here, and
add
> a
> > > NOTIMPLEMENTED(), like the other Create() functions above.
> > 
> > Done.
> > 
> > FYI: ImmersiveFullscreenController doesn't have it.
> 
> ImmersiveFullscreenController lives in //ash/shared, which means it has been
> ported to mustash.
> 

Sorry I misread the code.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/chro...
File ash/common/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/chro...
ash/common/system/chromeos/settings/tray_settings.cc:146: return nullptr;
On 2016/09/23 16:32:28, James Cook wrote:
> FYI, this is fine, but if you're worried about patch failures while
backporting
> I'm also OK with leaving these as NULL. Generally I only convert NULL ->
nullptr
> if I'm editing the line for another reason (like clang-format rewrapping).

Acknowledged.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:131:
WmShell::Get()->AddActivationObserver(this);
On 2016/09/23 16:32:28, James Cook wrote:
> optional: DCHECK(tray_).
> 
> (I've started using DCHECK for preconditions for functions, but it's just a
> style thing.)

Done.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/system_tray.h (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/system_tray.h:183: // Creates the key event watcher.
On 2016/09/23 16:32:28, James Cook wrote:
> nit: "See ShowItems() for why key events are observed."

Done.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:59: window->GetType() ==
ui::wm::WINDOW_TYPE_POPUP &&
On 2016/09/23 16:32:28, James Cook wrote:
> Any particular reason you're limiting this to always-on-top popup windows? Is
it
> because of clicking on the status area widget?

The original code simply ignored all widgets in status container, and assumes
that they'r either status area widgets or popup notification. Clicking widgets
in status area closes other bubbles, so I believe that exclusion wasn't
necessary. I narrowed the condition to be more specific to match popup
notifications.

Alternative approaches are:
1) Set and check the widget name
2) Introduce ash level window/widget type.

let me know which way you prefr or have better idea.

https://codereview.chromium.org/2330403002/diff/280001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:369: // crbug.com/
On 2016/09/23 16:32:28, James Cook wrote:
> Bug number please.

Done.

[oshima, 2016-09-23 19:20:05]

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

[commit-bot: I haz the power, 2016-09-23 19:20:58]

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, 2016-09-23 19:23:53]

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

[commit-bot: I haz the power, 2016-09-23 19:23:54]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[oshima, 2016-09-23 19:23:58]

On 2016/09/23 15:26:30, dmazzoni wrote:
> lgtm
> 
> No concerns about the code.
> 
> You mention that it activates the system tray bubble if you
> open it with the accelerator, I'm assuming you mean
> Alt+Shift+S.
> 
> There's another way to activate it: Press Ctrl+F1 until the
> system tray in the lower-right corner is focused, then press
> Enter to open it. Does this activate it as well, or
> essentially does keyboard access work well?

Thanks. I updated SystemTray::PerformAction to activate when keyboard is used.

> 
> No concerns about landing it first. Let's test it when it
> hits dev channel and watch out for regressions.

[oshima, 2016-09-23 19:25:52]

Patchset #5 (id:300001) has been deleted

[oshima, 2016-09-23 19:28:06]

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

[commit-bot: I haz the power, 2016-09-23 19:28:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[James Cook, 2016-09-23 19:41:02]

LGTM with nit. Nice patch -- it's always been odd that this "menu" activated
itself.

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2330403002/diff/280001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:59: window->GetType() ==
ui::wm::WINDOW_TYPE_POPUP &&
On 2016/09/23 19:19:54, oshima (OOO till Sep. 27) wrote:
> On 2016/09/23 16:32:28, James Cook wrote:
> > Any particular reason you're limiting this to always-on-top popup windows?
Is
> it
> > because of clicking on the status area widget?
> 
> The original code simply ignored all widgets in status container, and assumes
> that they'r either status area widgets or popup notification. Clicking widgets
> in status area closes other bubbles, so I believe that exclusion wasn't
> necessary. I narrowed the condition to be more specific to match popup
> notifications.
> 
> Alternative approaches are:
> 1) Set and check the widget name
> 2) Introduce ash level window/widget type.
> 
> let me know which way you prefr or have better idea.

I'm OK with what you have now.

https://codereview.chromium.org/2330403002/diff/320001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2330403002/diff/320001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:59: LOG(ERROR) << "Status
Container";
Remove this.

[dmazzoni, 2016-09-23 19:48:59]

On Fri, Sep 23, 2016 at 12:23 PM <oshima@chromium.org> wrote:

> > There's another way to activate it: Press Ctrl+F1 until the
> > system tray in the lower-right corner is focused, then press
> > Enter to open it. Does this activate it as well, or
> > essentially does keyboard access work well?
>
> Thanks. I updated SystemTray::PerformAction to activate when keyboard is
> used.
>

Great.

What about mixed mouse / keyboard? Open the tray bubble with the mouse,
then Tab to the volume control, then use the arrow keys to adjust it.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-09-23 20:58:33]

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

[commit-bot: I haz the power, 2016-09-23 20:58:34]

Dry run: This issue passed the CQ dry run.

[oshima, 2016-09-23 23:05:43]

On 2016/09/23 19:48:59, dmazzoni wrote:
> On Fri, Sep 23, 2016 at 12:23 PM <mailto:oshima@chromium.org> wrote:
> 
> > > There's another way to activate it: Press Ctrl+F1 until the
> > > system tray in the lower-right corner is focused, then press
> > > Enter to open it. Does this activate it as well, or
> > > essentially does keyboard access work well?
> >
> > Thanks. I updated SystemTray::PerformAction to activate when keyboard is
> > used.
> >
> 
> Great.
> 
> What about mixed mouse / keyboard? Open the tray bubble with the mouse,
> then Tab to the volume control, then use the arrow keys to adjust it.
> 

tab/alt-tab activates the bubble, so yes it does work.

> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[oshima, 2016-09-23 23:06:06]

https://codereview.chromium.org/2330403002/diff/320001/ash/common/system/tray...
File ash/common/system/tray/tray_event_filter.cc (right):

https://codereview.chromium.org/2330403002/diff/320001/ash/common/system/tray...
ash/common/system/tray/tray_event_filter.cc:59: LOG(ERROR) << "Status
Container";
On 2016/09/23 19:41:02, James Cook wrote:
> Remove this.

Done.

[oshima, 2016-09-23 23:07:03]

The CQ bit was checked by oshima@chromium.org

[oshima, 2016-09-23 23:07:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, dmazzoni@chromium.org, jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2330403002/#ps340001
(title: "Do not activate system tray bubble by default")

[commit-bot: I haz the power, 2016-09-23 23:07:28]

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, 2016-09-24 00:03:38]

Description was changed from

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated. I'll revisit this after this CL as going through all notification
is a huge task.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated. I'll revisit this after this CL as going through all notification
is a huge task.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.
==========

[commit-bot: I haz the power, 2016-09-24 00:03:40]

Committed patchset #6 (id:340001)

[commit-bot: I haz the power, 2016-09-24 00:07:02]

Description was changed from

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated. I'll revisit this after this CL as going through all notification
is a huge task.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.
==========

to

==========
Do not activate system tray bubble by default

* Open SystemTrayBubble in inactive state by default,
  and explicitly close the bubble after executing action.
* SystemTray::ActivationObserver to close the bubble if
  a window other than system tray or popup notification is
  activated. I'll revisit this after this CL as going through all notification
is a huge task.
* Introduced KeyEventWatcher so that SystemTrayBubble
  can close,or start keyboard navigation on
  esc/tab/alt-tab.
* Explicitly activate in following conditions
 - The focus is requested on the item.
 - Accelerator is used to open the bubble
   (for accessibility)

BUG=120680
TEST=manual. SystemTrayTest.Activation. Updated a few other tests to increase
coverage.

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

[commit-bot: I haz the power, 2016-09-24 00:07:04]

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