cros: Eliminate bluetooth methods from SystemTrayDelegate

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "ash/common/system/tray/tray_details_view.h"
 #include "ash/common/system/tray/tray_item_more.h"
 #include "ash/common/system/tray/tray_popup_item_style.h"
 #include "ash/common/system/tray/tray_popup_utils.h"
 #include "ash/common/system/tray/tri_view.h"
 #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"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/controls/button/toggle_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
@@ skipping 76 matching lines @@
 const int kDisabledPanelLabelBaselineY = 20;
 
 }  // namespace
 
 class BluetoothDefaultView : public TrayItemMore {
  public:
   explicit BluetoothDefaultView(SystemTrayItem* owner) : TrayItemMore(owner) {}
   ~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);
     } else {
       SetVisible(false);
     }
     UpdateStyle();
   }
 
  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);
 
     return style;
   }
 
   void UpdateStyle() override {
     TrayItemMore::UpdateStyle();
     std::unique_ptr<TrayPopupItemStyle> style = CreateStyle();
     SetImage(gfx::CreateVectorIcon(GetCurrentIcon(), style->GetIconColor()));
   }
 
  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;
         break;
       }
     }
     return has_connected_device ? kSystemMenuBluetoothConnectedIcon
                                 : kSystemMenuBluetoothIcon;
   }
 
@@ skipping 26 matching lines @@
     Layout();
   }
 
  private:
   void CreateItems() {
     CreateScrollableList();
     CreateTitleRow(IDS_ASH_STATUS_TRAY_BLUETOOTH);
   }
 
   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();
     }
   }
 
   void UpdateBluetoothDeviceList() {
     std::set<std::string> new_connecting_devices;
     std::set<std::string> new_connected_devices;
     std::set<std::string> new_paired_not_connected_devices;
     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);
         UpdateBluetoothDeviceListHelper(&connecting_devices_, list[i]);
       } else if (list[i].connected && list[i].paired) {
         new_connected_devices.insert(list[i].address);
         UpdateBluetoothDeviceListHelper(&connected_devices_, list[i]);
       } else if (list[i].paired) {
         new_paired_not_connected_devices.insert(list[i].address);
         UpdateBluetoothDeviceListHelper(&paired_not_connected_devices_,
                                         list[i]);
       } else {
         new_discovered_not_paired_devices.insert(list[i].address);
         UpdateBluetoothDeviceListHelper(&discovered_not_paired_devices_,
                                         list[i]);
       }
     }
     RemoveObsoleteBluetoothDevicesFromList(&connecting_devices_,
                                            new_connecting_devices);
     RemoveObsoleteBluetoothDevicesFromList(&connected_devices_,
                                            new_connected_devices);
     RemoveObsoleteBluetoothDevicesFromList(&paired_not_connected_devices_,
                                            new_paired_not_connected_devices);
     RemoveObsoleteBluetoothDevicesFromList(&discovered_not_paired_devices_,
                                            new_discovered_not_paired_devices);
   }
 
   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);
   }
 
   void UpdateDeviceScrollList() {
     std::string focused_device_address = GetFocusedDeviceAddress();
 
     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.
     if (bluetooth_enabled) {
       HideDisabledPanel();
     } else {
       ShowDisabledPanel();
       return;
     }
 
@@ skipping 138 matching lines @@
       gfx::ImageSkia icon_image = CreateVectorIcon(
           GetBluetoothDeviceIcon(device_type, false), style.GetIconColor());
       SetupConnectingItem(container, display_name, icon_image);
       scroll_content()->SizeToPreferredSize();
       scroller()->Layout();
     }
   }
 
   // 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;
     find = device_map_.find(view);
     if (find == device_map_.end())
       return;
 
     const std::string device_id = find->second;
     if (FoundDevice(device_id, connecting_devices_, nullptr, nullptr))
       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 {
       NOTREACHED();
     }
   }
 
   void CreateExtraTitleRowButtons() override {
     if (login_ == LoginStatus::LOCKED)
       return;
@@ skipping 136 matching lines @@
 
 views::View* TrayBluetooth::CreateDefaultView(LoginStatus status) {
   CHECK(default_ == NULL);
   default_ = new tray::BluetoothDefaultView(this);
   default_->SetEnabled(status != LoginStatus::LOCKED);
   default_->Update();
   return default_;
 }
 
 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);
   CHECK(detailed_ == NULL);
   detailed_ = new tray::BluetoothDetailedView(this, status);
   detailed_->Update();
   return detailed_;
 }
 
 void TrayBluetooth::DestroyTrayView() {}
@@ skipping 15 matching lines @@
     detailed_->Update();
 }
 
 void TrayBluetooth::OnBluetoothDiscoveringChanged() {
   if (!detailed_)
     return;
   detailed_->Update();
 }
 
 }  // namespace ash