cros: Eliminate bluetooth methods from SystemTrayDelegate

ash/common/system/chromeos/bluetooth/tray_bluetooth.cc

Index: ash/common/system/chromeos/bluetooth/tray_bluetooth.cc
diff --git a/ash/common/system/chromeos/bluetooth/tray_bluetooth.cc b/ash/common/system/chromeos/bluetooth/tray_bluetooth.cc
index 6eb6aac83fbb3f9ac7680dd7503c6f7f251a44e9..8ccf1bff37b37580a43099c61b4078aad5a268a5 100644
--- a/ash/common/system/chromeos/bluetooth/tray_bluetooth.cc
+++ b/ash/common/system/chromeos/bluetooth/tray_bluetooth.cc
@@ -5,10 +5,10 @@
 #include "ash/common/system/chromeos/bluetooth/tray_bluetooth.h"
 
 #include "ash/common/session/session_state_delegate.h"
+#include "ash/common/system/chromeos/bluetooth/tray_bluetooth_helper.h"
 #include "ash/common/system/tray/hover_highlight_view.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/system_tray_controller.h"
-#include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/system_tray_notifier.h"
 #include "ash/common/system/tray/throbber_view.h"
 #include "ash/common/system/tray/tray_constants.h"
@@ -20,6 +20,7 @@
 #include "ash/common/wm_shell.h"
 #include "ash/resources/grit/ash_resources.h"
 #include "ash/resources/vector_icons/vector_icons.h"
+#include "ash/shell.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "device/bluetooth/bluetooth_common.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -116,13 +117,13 @@ class BluetoothDefaultView : public TrayItemMore {
   ~BluetoothDefaultView() override {}
 
   void Update() {
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
-    const bool enabled = delegate->GetBluetoothEnabled();
-    if (delegate->GetBluetoothAvailable()) {
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
+    if (helper->GetBluetoothAvailable()) {
       ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-      const base::string16 label = rb.GetLocalizedString(
-          enabled ? IDS_ASH_STATUS_TRAY_BLUETOOTH_ENABLED
-                  : IDS_ASH_STATUS_TRAY_BLUETOOTH_DISABLED);
+      const base::string16 label =
+          rb.GetLocalizedString(helper->GetBluetoothEnabled()
+                                    ? IDS_ASH_STATUS_TRAY_BLUETOOTH_ENABLED
+                                    : IDS_ASH_STATUS_TRAY_BLUETOOTH_DISABLED);
       SetLabel(label);
       SetAccessibleName(label);
       SetVisible(true);
@@ -135,13 +136,13 @@ class BluetoothDefaultView : public TrayItemMore {
  protected:
   // TrayItemMore:
   std::unique_ptr<TrayPopupItemStyle> HandleCreateStyle() const override {
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
     std::unique_ptr<TrayPopupItemStyle> style =
         TrayItemMore::HandleCreateStyle();
     style->set_color_style(
-        delegate->GetBluetoothEnabled()
+        helper->GetBluetoothEnabled()
             ? TrayPopupItemStyle::ColorStyle::ACTIVE
-            : delegate->GetBluetoothAvailable()
+            : helper->GetBluetoothAvailable()
                   ? TrayPopupItemStyle::ColorStyle::INACTIVE
                   : TrayPopupItemStyle::ColorStyle::DISABLED);
 
@@ -156,13 +157,13 @@ class BluetoothDefaultView : public TrayItemMore {
 
  private:
   const gfx::VectorIcon& GetCurrentIcon() {
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
-    if (!delegate->GetBluetoothEnabled())
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
+    if (!helper->GetBluetoothEnabled())
       return kSystemMenuBluetoothDisabledIcon;
 
     bool has_connected_device = false;
     BluetoothDeviceList list;
-    delegate->GetAvailableBluetoothDevices(&list);
+    helper->GetAvailableBluetoothDevices(&list);
     for (size_t i = 0; i < list.size(); ++i) {
       if (list[i].connected) {
         has_connected_device = true;
@@ -209,20 +210,20 @@ class BluetoothDetailedView : public TrayDetailsView {
   }
 
   void BluetoothStartDiscovering() {
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
-    if (delegate->GetBluetoothDiscovering()) {
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
+    if (helper->HasBluetoothDiscoverySession()) {

[msw, 2017/03/21 01:54:43]

Did you figure out if there's a meaningful difference between
GetBluetoothDiscovering() and HasBluetoothDiscoverySession()? I wonder if Get
returns true immediately once the discovery session is requested, and Has only
returns true later, when the discovery session has been asynchronously started
and is claimed by the helper (ie. in
TrayBluetoothHelper::OnStartDiscoverySession()).

Consider leaving these two GetBluetoothDiscovering() calls as they were, or
pinging a more knowledgeable bluetooth reviewer to see if this introduces a
non-trivial behavior change. (or maybe you know this change is fine as-is)

[James Cook, 2017/03/21 04:43:01]

This isn't a behavior change.
TrayBluetoothHelper::HasBluetoothDiscoverySession() is equivalent to
SystemTrayDelegateChromeOS::GetBluetoothDiscovering(). The names are terrible.
At least the new name tells you what the call is doing.

It turns out the other method IsBluetoothDiscovering() wasn't used, so I removed
it.

       ShowLoadingIndicator();
       return;
     }
     HideLoadingIndicator();
-    if (delegate->GetBluetoothEnabled())
-      delegate->BluetoothStartDiscovering();
+    if (helper->GetBluetoothEnabled())
+      helper->StartBluetoothDiscovering();
   }
 
   void BluetoothStopDiscovering() {
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
-    if (delegate && delegate->GetBluetoothDiscovering()) {
-      delegate->BluetoothStopDiscovering();
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
+    if (helper && helper->HasBluetoothDiscoverySession()) {

[msw, 2017/03/21 01:54:43]

nit: maybe the null check isn't needed anymore? (not done elsewhere)

[James Cook, 2017/03/21 04:43:01]

I think this avoids a shutdown crash if the system tray menu is open during
sign-out/shutdown (BluetoothStopDiscovering() is called in the destructor).

I looked at adding an explicit TrayBluetoothHelper::Shutdown() phase, but that
seems worse, since it means there's a new mostly-shut-down state the helper can
be in.

Let me know if you think an explicit Shutdown() would be better and I'll do it
in a follow-up.

+      helper->StopBluetoothDiscovering();
       HideLoadingIndicator();
     }
   }
@@ -234,7 +235,7 @@ class BluetoothDetailedView : public TrayDetailsView {
     std::set<std::string> new_discovered_not_paired_devices;
 
     BluetoothDeviceList list;
-    WmShell::Get()->system_tray_delegate()->GetAvailableBluetoothDevices(&list);
+    Shell::Get()->tray_bluetooth_helper()->GetAvailableBluetoothDevices(&list);
     for (size_t i = 0; i < list.size(); ++i) {
       if (list[i].connecting) {
         new_connecting_devices.insert(list[i].address);
@@ -263,8 +264,8 @@ class BluetoothDetailedView : public TrayDetailsView {
   }
 
   void UpdateHeaderEntry() {
-    bool is_bluetooth_enabled =
-        WmShell::Get()->system_tray_delegate()->GetBluetoothEnabled();
+    const bool is_bluetooth_enabled =
+        Shell::Get()->tray_bluetooth_helper()->GetBluetoothEnabled();
     if (toggle_)
       toggle_->SetIsOn(is_bluetooth_enabled, true);
   }
@@ -275,9 +276,9 @@ class BluetoothDetailedView : public TrayDetailsView {
     device_map_.clear();
     scroll_content()->RemoveAllChildViews(true);
 
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
-    bool bluetooth_enabled = delegate->GetBluetoothEnabled();
-    bool bluetooth_available = delegate->GetBluetoothAvailable();
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
+    const bool bluetooth_enabled = helper->GetBluetoothEnabled();
+    const bool bluetooth_available = helper->GetBluetoothAvailable();
 
     // If Bluetooth is disabled, show a panel which only indicates that it is
     // disabled, instead of the scroller with Bluetooth devices.
@@ -436,8 +437,8 @@ class BluetoothDetailedView : public TrayDetailsView {
 
   // TrayDetailsView:
   void HandleViewClicked(views::View* view) override {
-    SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
-    if (!delegate->GetBluetoothEnabled())
+    TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
+    if (!helper->GetBluetoothEnabled())
       return;
 
     std::map<views::View*, std::string>::iterator find;
@@ -450,17 +451,17 @@ class BluetoothDetailedView : public TrayDetailsView {
       return;
 
     UpdateClickedDevice(device_id, view);
-    delegate->ConnectToBluetoothDevice(device_id);
+    helper->ConnectToBluetoothDevice(device_id);
   }
 
   void HandleButtonPressed(views::Button* sender,
                            const ui::Event& event) override {
     if (sender == toggle_) {
-      SystemTrayDelegate* delegate = WmShell::Get()->system_tray_delegate();
+      TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
       WmShell::Get()->RecordUserMetricsAction(
-          delegate->GetBluetoothEnabled() ? UMA_STATUS_AREA_BLUETOOTH_DISABLED
-                                          : UMA_STATUS_AREA_BLUETOOTH_ENABLED);
-      delegate->ToggleBluetooth();
+          helper->GetBluetoothEnabled() ? UMA_STATUS_AREA_BLUETOOTH_DISABLED
+                                        : UMA_STATUS_AREA_BLUETOOTH_ENABLED);
+      helper->ToggleBluetoothEnabled();
     } else if (sender == settings_) {
       ShowSettings();
     } else {
@@ -617,7 +618,7 @@ views::View* TrayBluetooth::CreateDefaultView(LoginStatus status) {
 }
 
 views::View* TrayBluetooth::CreateDetailedView(LoginStatus status) {
-  if (!WmShell::Get()->system_tray_delegate()->GetBluetoothAvailable())
+  if (!Shell::Get()->tray_bluetooth_helper()->GetBluetoothAvailable())
     return NULL;
   WmShell::Get()->RecordUserMetricsAction(
       UMA_STATUS_AREA_DETAILED_BLUETOOTH_VIEW);