Do not activate system tray bubble by default

ash/common/system/tray/system_tray.cc

Index: ash/common/system/tray/system_tray.cc
diff --git a/ash/common/system/tray/system_tray.cc b/ash/common/system/tray/system_tray.cc
index a03af66b890b667921b23c15b7bfbd5463e16e09..bdd25e31d7c80ddf354ecfe4e670ea3d76331e23 100644
--- a/ash/common/system/tray/system_tray.cc
+++ b/ash/common/system/tray/system_tray.cc
@@ -4,6 +4,7 @@
 
 #include "ash/common/system/tray/system_tray.h"
 
+#include "ash/common/key_event_watcher.h"
 #include "ash/common/login_status.h"
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/session/session_state_delegate.h"
@@ -30,6 +31,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/timer/timer.h"
 #include "grit/ash_strings.h"
+#include "ui/base/accelerators/accelerator.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/compositor/layer.h"
 #include "ui/display/display.h"
@@ -64,6 +66,8 @@
 #include "ui/message_center/message_center.h"
 #endif
 
+#include "base/debug/stack_trace.h"

[James Cook, 2016/09/22 21:17:16]

remove

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

Oops, done.

+
 using views::TrayBubbleView;
 
 namespace ash {
@@ -140,6 +144,7 @@ SystemTray::SystemTray(WmShelf* wm_shelf)
 
 SystemTray::~SystemTray() {
   // Destroy any child views that might have back pointers before ~View().
+  key_event_watcher_.reset();
   system_bubble_.reset();
   notification_bubble_.reset();
   for (std::vector<SystemTrayItem*>::iterator it = items_.begin();
@@ -159,6 +164,7 @@ void SystemTray::InitializeTrayItems(
 
 void SystemTray::Shutdown() {
   DCHECK(web_notification_tray_);
+  key_event_watcher_.reset();

[James Cook, 2016/09/22 21:17:16]

Does this need to be both here and in destructor?

[oshima, 2016/09/23 09:37:19]

Yes, it is in dtor too.

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

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.

[oshima, 2016/09/23 19:19:53]

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

   web_notification_tray_ = nullptr;
 }
 
@@ -498,7 +504,8 @@ void SystemTray::ShowItems(const std::vector<SystemTrayItem*>& items,
     TrayBubbleView::InitParams init_params(TrayBubbleView::ANCHOR_TYPE_TRAY,
                                            GetAnchorAlignment(), menu_width,
                                            kTrayPopupMaxWidth);
-    init_params.can_activate = can_activate;
+    // TODO(oshima): Change TrayBubbleView itself.
+    init_params.can_activate = false;
     init_params.first_item_has_no_margin = true;
     if (detailed) {
       // This is the case where a volume control or brightness control bubble
@@ -529,6 +536,22 @@ void SystemTray::ShowItems(const std::vector<SystemTrayItem*>& items,
   if (!detailed)
     default_bubble_height_ = system_bubble_->bubble_view()->height();
 
+  key_event_watcher_.reset();

[James Cook, 2016/09/22 21:17:16]

optional: This might be clearer in an else{} clause below.

[oshima, 2016/09/23 09:37:19]

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)

+  if (can_activate) {

[James Cook, 2016/09/22 21:17:16]

Document what you are doing in this block of code and/or split into a separate
function, since ShowItems() is > 100 lines long now.

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

Done.

+    key_event_watcher_ = WmShell::Get()->CreateKeyEventWatcher();
+    key_event_watcher_->AddKeyEventCallback(
+        ui::Accelerator(ui::VKEY_ESCAPE, 0),

[James Cook, 2016/09/22 21:17:16]

0 -> EF_NONE

[oshima, 2016/09/23 09:37:19]

Done.

+        base::Bind(&SystemTray::CloseBubble, base::Unretained(this)));
+    key_event_watcher_->AddKeyEventCallback(
+        ui::Accelerator(ui::VKEY_TAB, 0),
+        base::Bind(&SystemTray::ActivateAndStartNavigation,
+                   base::Unretained(this)));
+    key_event_watcher_->AddKeyEventCallback(
+        ui::Accelerator(ui::VKEY_TAB, ui::EF_SHIFT_DOWN),
+        base::Bind(&SystemTray::ActivateAndStartNavigation,
+                   base::Unretained(this)));
+  }
+
   if (detailed && items.size() > 0)
     detailed_item_ = items[0];
   else
@@ -731,6 +754,24 @@ TrayUpdate* SystemTray::GetTrayUpdateForTesting() const {
   return tray_update_;
 }
 
+void SystemTray::CloseBubble(const ui::KeyEvent& key_event) {
+  CloseSystemBubble();
+}
+
+void SystemTray::ActivateAndStartNavigation(const ui::KeyEvent& key_event) {
+  if (!system_bubble_)
+    return;
+  ActivateBubble();
+  views::Widget* widget = GetSystemBubble()->bubble_view()->GetWidget();
+  widget->GetFocusManager()->OnKeyEvent(key_event);
+}
+
+void SystemTray::ActivateBubble() {
+  TrayBubbleView* bubble_view = GetSystemBubble()->bubble_view();
+  bubble_view->set_can_activate(true);
+  bubble_view->GetWidget()->Activate();
+}
+
 bool SystemTray::PerformAction(const ui::Event& event) {
   // If we're already showing the default view, hide it; otherwise, show it
   // (and hide any popup that's currently shown).
@@ -753,6 +794,7 @@ bool SystemTray::PerformAction(const ui::Event& event) {
 }
 
 void SystemTray::CloseSystemBubbleAndDeactivateSystemTray() {
+  key_event_watcher_.reset();
   system_bubble_.reset();
   // When closing a system bubble with the alternate shelf layout, we need to
   // turn off the active tinting of the shelf.