Refactor AddScrollListItem() in system menu detailed views

ash/system/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/system/bluetooth/tray_bluetooth.h"
 
 #include "ash/resources/grit/ash_resources.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "ash/session/session_state_delegate.h"
 #include "ash/shell.h"
 #include "ash/shell_port.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/system/bluetooth/tray_bluetooth_helper.h"
 #include "ash/system/tray/hover_highlight_view.h"
 #include "ash/system/tray/system_tray.h"
 #include "ash/system/tray/system_tray_controller.h"
 #include "ash/system/tray/system_tray_notifier.h"
 #include "ash/system/tray/throbber_view.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/tray_details_view.h"
 #include "ash/system/tray/tray_item_more.h"
 #include "ash/system/tray/tray_popup_item_style.h"
 #include "ash/system/tray/tray_popup_utils.h"
 #include "ash/system/tray/tri_view.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/gfx/vector_icon_types.h"
 #include "ui/views/controls/button/toggle_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/progress_bar.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
 
 namespace ash {
 namespace tray {
@@ skipping 71 matching lines @@
 }  // namespace
 
 class BluetoothDefaultView : public TrayItemMore {
  public:
   explicit BluetoothDefaultView(SystemTrayItem* owner) : TrayItemMore(owner) {}
   ~BluetoothDefaultView() override {}
 
   void Update() {
     TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
     if (helper->GetBluetoothAvailable()) {
-      ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance();
-      const base::string16 label =
-          rb.GetLocalizedString(helper->GetBluetoothEnabled()
-                                    ? IDS_ASH_STATUS_TRAY_BLUETOOTH_ENABLED
-                                    : IDS_ASH_STATUS_TRAY_BLUETOOTH_DISABLED);
+      const base::string16 label = l10n_util::GetStringUTF16(

[tdanderson, 2017/04/24 15:43:39]

Thanks for the extra little fixes such as this throughout the CL

+          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:
@@ skipping 19 matching lines @@
   }
 
  private:
   const gfx::VectorIcon& GetCurrentIcon() {
     TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
     if (!helper->GetBluetoothEnabled())
       return kSystemMenuBluetoothDisabledIcon;
 
     bool has_connected_device = false;
     BluetoothDeviceList list = helper->GetAvailableBluetoothDevices();
-    for (size_t i = 0; i < list.size(); ++i) {
-      if (list[i].connected) {
+    for (const auto& device : list) {
+      if (device.connected) {
         has_connected_device = true;
         break;
       }
     }
     return has_connected_device ? kSystemMenuBluetoothConnectedIcon
                                 : kSystemMenuBluetoothIcon;
   }
 
   DISALLOW_COPY_AND_ASSIGN(BluetoothDefaultView);
 };
@@ skipping 50 matching lines @@
   }
 
   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 =
         Shell::Get()->tray_bluetooth_helper()->GetAvailableBluetoothDevices();
-    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]);
+    for (const auto& device : list) {
+      if (device.connecting) {
+        new_connecting_devices.insert(device.address);
+        UpdateBluetoothDeviceListHelper(&connecting_devices_, device);
+      } else if (device.connected && device.paired) {
+        new_connected_devices.insert(device.address);
+        UpdateBluetoothDeviceListHelper(&connected_devices_, device);
+      } else if (device.paired) {
+        new_paired_not_connected_devices.insert(device.address);
+        UpdateBluetoothDeviceListHelper(&paired_not_connected_devices_, device);
       } else {
-        new_discovered_not_paired_devices.insert(list[i].address);
+        new_discovered_not_paired_devices.insert(device.address);
         UpdateBluetoothDeviceListHelper(&discovered_not_paired_devices_,
-                                        list[i]);
+                                        device);
       }
     }
     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);
@@ skipping 23 matching lines @@
     } else {
       ShowDisabledPanel();
       return;
     }
 
     // Add paired devices and their section header to the list.
     size_t num_paired_devices = connected_devices_.size() +
                                 connecting_devices_.size() +
                                 paired_not_connected_devices_.size();
     if (num_paired_devices > 0) {
-      AddSubHeader(IDS_ASH_STATUS_TRAY_BLUETOOTH_PAIRED_DEVICES);
+      const base::string16 text = l10n_util::GetStringUTF16(
+          IDS_ASH_STATUS_TRAY_BLUETOOTH_PAIRED_DEVICES);
+      AddScrollListSubHeader(gfx::kNoneIcon, text);
       AppendSameTypeDevicesToScrollList(connected_devices_, true, true,
                                         bluetooth_enabled);
       AppendSameTypeDevicesToScrollList(connecting_devices_, true, false,
                                         bluetooth_enabled);
       AppendSameTypeDevicesToScrollList(paired_not_connected_devices_, false,
                                         false, bluetooth_enabled);
     }
 
     // Add unpaired devices to the list. If at least one paired device is
     // present, also add a section header above the unpaired devices.
     if (discovered_not_paired_devices_.size() > 0) {
-      if (num_paired_devices > 0)
-        AddSubHeader(IDS_ASH_STATUS_TRAY_BLUETOOTH_UNPAIRED_DEVICES);
+      if (num_paired_devices > 0) {
+        const base::string16 text = l10n_util::GetStringUTF16(
+            IDS_ASH_STATUS_TRAY_BLUETOOTH_UNPAIRED_DEVICES);
+        AddScrollListSubHeader(gfx::kNoneIcon, text);
+      }
       AppendSameTypeDevicesToScrollList(discovered_not_paired_devices_, false,
                                         false, bluetooth_enabled);
     }
 
     // Show user Bluetooth state if there is no bluetooth devices in list.
     if (device_map_.size() == 0 && bluetooth_available && bluetooth_enabled) {
       scroll_content()->AddChildView(TrayPopupUtils::CreateInfoLabelRowView(
           IDS_ASH_STATUS_TRAY_BLUETOOTH_DISCOVERING));
     }
 
     // Focus the device which was focused before the device-list update.
     if (!focused_device_address.empty())
       FocusDeviceByAddress(focused_device_address);
 
     scroll_content()->InvalidateLayout();
   }
 
   void AppendSameTypeDevicesToScrollList(const BluetoothDeviceList& list,
                                          bool highlight,
                                          bool checked,
                                          bool enabled) {
-    for (size_t i = 0; i < list.size(); ++i) {
-      HoverHighlightView* container = nullptr;
-      gfx::ImageSkia icon_image = CreateVectorIcon(
-          GetBluetoothDeviceIcon(list[i].device_type, list[i].connected),
-          kMenuIconColor);
-      container = AddScrollListItem(list[i].display_name, icon_image,
-                                    list[i].connected, list[i].connecting);
-      device_map_[container] = list[i].address;
+    for (const auto& device : list) {
+      const gfx::VectorIcon& icon =
+          GetBluetoothDeviceIcon(device.device_type, device.connected);
+      HoverHighlightView* container =
+          AddScrollListItem(icon, device.display_name);
+      if (device.connected)
+        SetupConnectedItem(container);
+      else if (device.connecting)
+        SetupConnectingItem(container);
+      device_map_[container] = device.address;
     }
   }
 
-  HoverHighlightView* AddScrollListItem(const base::string16& text,
-                                        const gfx::ImageSkia& image,
-                                        bool connected,
-                                        bool connecting) {
-    HoverHighlightView* container = new HoverHighlightView(this);
-    if (connected) {
-      SetupConnectedItem(container, text, image);
-    } else if (connecting) {
-      SetupConnectingItem(container, text, image);
-    } else {
-      container->AddIconAndLabel(image, text);
-    }
-    scroll_content()->AddChildView(container);
-    return container;
-  }
-
-  void AddSubHeader(int message_id) {
-    TriView* header = TrayPopupUtils::CreateSubHeaderRowView();
-    TrayPopupUtils::ConfigureAsStickyHeader(header);
-
-    views::Label* label = TrayPopupUtils::CreateDefaultLabel();
-    label->SetText(l10n_util::GetStringUTF16(message_id));
-    TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::SUB_HEADER);
-    style.SetupLabel(label);
-    header->AddView(TriView::Container::CENTER, label);
-
-    scroll_content()->AddChildView(header);
-  }
-
-  void SetupConnectedItem(HoverHighlightView* container,
-                          const base::string16& text,
-                          const gfx::ImageSkia& image) {
-    container->AddIconAndLabels(
-        image, text, l10n_util::GetStringUTF16(
-                         IDS_ASH_STATUS_TRAY_NETWORK_STATUS_CONNECTED));
+  void SetupConnectedItem(HoverHighlightView* container) {
+    container->SetSubText(l10n_util::GetStringUTF16(
+        IDS_ASH_STATUS_TRAY_NETWORK_STATUS_CONNECTED));
     TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::CAPTION);
     style.set_color_style(TrayPopupItemStyle::ColorStyle::CONNECTED);
     style.SetupLabel(container->sub_text_label());
   }
 
-  void SetupConnectingItem(HoverHighlightView* container,
-                           const base::string16& text,
-                           const gfx::ImageSkia& image) {
-    container->AddIconAndLabels(
-        image, text, l10n_util::GetStringUTF16(
-                         IDS_ASH_STATUS_TRAY_NETWORK_STATUS_CONNECTING));
+  void SetupConnectingItem(HoverHighlightView* container) {
+    container->SetSubText(l10n_util::GetStringUTF16(
+        IDS_ASH_STATUS_TRAY_NETWORK_STATUS_CONNECTING));
     ThrobberView* throbber = new ThrobberView;
     throbber->Start();
     container->AddRightView(throbber);
   }
 
-  // Returns true if the device with |device_id| is found in |device_list|,
-  // and the display_name of the device will be returned in |display_name| if
-  // it's not NULL.
+  // Returns true if the device with |device_id| is found in |device_list|.
   bool FoundDevice(const std::string& device_id,
-                   const BluetoothDeviceList& device_list,
-                   base::string16* display_name,
-                   device::BluetoothDeviceType* device_type) {
-    for (size_t i = 0; i < device_list.size(); ++i) {
-      if (device_list[i].address == device_id) {
-        if (display_name)

[tdanderson, 2017/04/24 15:43:39]

I am slightly uneasy about removing this logic along with your changes to
UpdateClickedDevice(). Is it at all possible for a device's display name or type
to change after having been added to the list? If so then I think you may need
to keep some of this extra logic here.

Consider removing these changes to a small follow-up CL and have fukino@
(Bluetooth owner) take a look.

[mohsen, 2017/04/25 05:10:01]

If a device name is changed, it would be updated in
BluetoothDetailedView::Update(). UpdateclickedDevice() is called when an item is
clicked and is only responsible for updating connecting/connected messages. I
think the only reason it also updates name and icon is that it uses
SetupConnectingItem() which sets up the whole row from scratch. Does this make
sense? If not, I can exclude changes in tray_bluetooth from this Cl as you
suggested.

[tdanderson, 2017/04/25 14:32:48]

OK, that makes sense. I'd be ok landing as-is. But in the next patch set please
cc fukino@ just as an FYI to these changes.

-          *display_name = device_list[i].display_name;
-        if (device_type)
-          *device_type = device_list[i].device_type;
+                   const BluetoothDeviceList& device_list) {
+    for (const auto& device : device_list) {
+      if (device.address == device_id)
         return true;
-      }
     }
     return false;
   }
 
   // Updates UI of the clicked bluetooth device to show it is being connected
   // or disconnected if such an operation is going to be performed underway.
   void UpdateClickedDevice(const std::string& device_id,
                            views::View* item_container) {
-    base::string16 display_name;
-    device::BluetoothDeviceType device_type;
-    if (FoundDevice(device_id, paired_not_connected_devices_, &display_name,
-                    &device_type)) {
-      item_container->RemoveAllChildViews(true);
+    if (FoundDevice(device_id, paired_not_connected_devices_)) {
       HoverHighlightView* container =
           static_cast<HoverHighlightView*>(item_container);
-      TrayPopupItemStyle style(
-          TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
-      gfx::ImageSkia icon_image = CreateVectorIcon(
-          GetBluetoothDeviceIcon(device_type, false), style.GetIconColor());
-      SetupConnectingItem(container, display_name, icon_image);
+      SetupConnectingItem(container);
       scroll_content()->SizeToPreferredSize();
       scroller()->Layout();
     }
   }
 
   // TrayDetailsView:
   void HandleViewClicked(views::View* view) override {
     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))
+    if (FoundDevice(device_id, connecting_devices_))
       return;
 
     UpdateClickedDevice(device_id, view);
     helper->ConnectToBluetoothDevice(device_id);
   }
 
   void HandleButtonPressed(views::Button* sender,
                            const ui::Event& event) override {
     if (sender == toggle_) {
       TrayBluetoothHelper* helper = Shell::Get()->tray_bluetooth_helper();
@@ skipping 73 matching lines @@
         TrayPopupItemStyle::FontStyle::DETAILED_VIEW_LABEL);
     style.set_color_style(TrayPopupItemStyle::ColorStyle::DISABLED);
 
     views::ImageView* image_view = new views::ImageView;
     image_view->SetImage(gfx::CreateVectorIcon(kSystemMenuBluetoothDisabledIcon,
                                                style.GetIconColor()));
     image_view->SetVerticalAlignment(views::ImageView::TRAILING);
     container->AddChildView(image_view);
 
     views::Label* label = new views::Label(
-        ui::ResourceBundle::GetSharedInstance().GetLocalizedString(
-            IDS_ASH_STATUS_TRAY_BLUETOOTH_DISABLED));
+        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_BLUETOOTH_DISABLED));
     style.SetupLabel(label);
     label->SetBorder(views::CreateEmptyBorder(
         kDisabledPanelLabelBaselineY - label->GetBaseline(), 0, 0, 0));
     container->AddChildView(label);
 
     // Make top padding of the icon equal to the height of the label so that the
     // icon is vertically aligned to center of the container.
     image_view->SetBorder(
         views::CreateEmptyBorder(label->GetPreferredSize().height(), 0, 0, 0));
     return container;
@@ skipping 91 matching lines @@
     detailed_->Update();
 }
 
 void TrayBluetooth::OnBluetoothDiscoveringChanged() {
   if (!detailed_)
     return;
   detailed_->Update();
 }
 
 }  // namespace ash