Refactor AddScrollListItem() in system menu detailed views

ash/system/audio/audio_detailed_view.cc

Index: ash/system/audio/audio_detailed_view.cc
diff --git a/ash/system/audio/audio_detailed_view.cc b/ash/system/audio/audio_detailed_view.cc
index 2bcfc8b00f17f867832598513a239a629c96dd79..b1192c9019e30016e75da57972eaffbf3ba1d2c8 100644
--- a/ash/system/audio/audio_detailed_view.cc
+++ b/ash/system/audio/audio_detailed_view.cc
@@ -14,9 +14,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
-#include "ui/native_theme/native_theme.h"
 #include "ui/views/controls/image_view.h"
-#include "ui/views/controls/label.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/controls/separator.h"
 
@@ -73,47 +71,11 @@ void AudioDetailedView::Update() {
   Layout();
 }
 
-void AudioDetailedView::AddInputHeader() {
-  AddScrollListInfoItem(IDS_ASH_STATUS_TRAY_AUDIO_INPUT,
-                        kSystemMenuAudioInputIcon);
-}
-
-void AudioDetailedView::AddOutputHeader() {
-  AddScrollListInfoItem(IDS_ASH_STATUS_TRAY_AUDIO_OUTPUT,
-                        kSystemMenuAudioOutputIcon);
-}
-
-void AudioDetailedView::AddScrollListInfoItem(int text_id,
-                                              const gfx::VectorIcon& icon) {
-  TriView* header = TrayPopupUtils::CreateDefaultRowView();

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

Just wanted to double-check that this works as intended, since this function
calls CreateDefaultRowView() but its replacement (AddScrollListSubHeader()) is
calling CreateSubHeaderRowView().

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

Yes, different layout managers are installed with different paddings. However,
they produce very similar output for our purposes. I talked with bruthig@ and
decided that CreateSubHeaderRowView() should actually use CreateDefaultRoView()
instead of directly using CreateMultiTargetRowView() so that similar padding and
layout management is used for sub-header rows, too. Done. WDYT?

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

By 'very similar' do you mean that the implementation in Patch Set 4 will give
the incorrect layout for the audio subheader rows?
Unless the code in patch set 4 actually gives the incorrect layout, I would
prefer for the time being to avoid calling CreateDefaultRowView() directly from
CreateSubHeaderRowView() like you have in patch set 6. If there is an
opportunity to share code between the two then by all means create a private
helper function, but I see it as a bit confusing to call a function designed for
use by default views directly from a function designed for use by detailed
views.

I am also happy to table this discussion for a follow-on CL, provided that
whatever you're landing in this CL will still follow design specs.

[mohsen, 2017/04/25 19:23:47]

Yes, the label padding was 4 pixels off.
I don't think CreateDefaultRowView() is designed only for default views. It is
also used for rows in detailed views, as far as I can tell. And it seems
reasonable to me to use it for sub-header rows as we want icons and labels in
sub-headers and regular rows be aligned properly.

[tdanderson, 2017/04/25 21:00:27]

Yes you're right. I was reading the function as "create (default row) view"
rather than "create default (row view)". Doing this sgtm.

-  TrayPopupUtils::ConfigureAsStickyHeader(header);
-  views::ImageView* image_view = TrayPopupUtils::CreateMainImageView();
-  image_view->SetImage(gfx::CreateVectorIcon(
-      icon, GetNativeTheme()->GetSystemColor(
-                ui::NativeTheme::kColorId_ProminentButtonColor)));
-  header->AddView(TriView::Container::START, image_view);
-
-  views::Label* label = TrayPopupUtils::CreateDefaultLabel();
-  label->SetText(l10n_util::GetStringUTF16(text_id));
-  TrayPopupItemStyle style(TrayPopupItemStyle::FontStyle::SUB_HEADER);
-  style.SetupLabel(label);
-  header->AddView(TriView::Container::CENTER, label);
-
+void AudioDetailedView::AddAudioSubHeader(const gfx::VectorIcon& icon,
+                                          int text_id) {
+  const base::string16 text = l10n_util::GetStringUTF16(text_id);
+  TriView* header = AddScrollListSubHeader(icon, text);

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

Consider passing in the text_id to AddScrollListSubHeader instead of the string
itself, which will eliminate the need for each call site to call
l10_util::GetStringUTF16(). This assumes that all of our subheaders have (and
will have) a pre-defined string id, which is probably the case. WDYT?

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

Done. I wanted AddScrollListSubHeader() to be more consistent with
AddScrollListItem() which cannot take a resource id. But, your suggestion also
makes sense and I don't have any preference.

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

Ah I see, I guess we could have gone either way with this.

   header->SetContainerVisible(TriView::Container::END, false);
-  scroll_content()->AddChildView(header);
-}
-
-HoverHighlightView* AudioDetailedView::AddScrollListItem(
-    const base::string16& text,
-    bool highlight,
-    bool checked) {
-  HoverHighlightView* container = new HoverHighlightView(this);
-
-  container->AddLabelRow(text);
-  TrayPopupUtils::InitializeAsCheckableRow(container, checked);
-
-  scroll_content()->AddChildView(container);
-  return container;
 }
 
 void AudioDetailedView::CreateItems() {
@@ -129,21 +91,20 @@ void AudioDetailedView::UpdateAudioDevices() {
   audio_handler->GetAudioDevices(&devices);
   bool has_dual_internal_mic = audio_handler->HasDualInternalMic();
   bool is_front_or_rear_mic_active = false;
-  for (size_t i = 0; i < devices.size(); ++i) {
+  for (const auto& device : devices) {
     // Don't display keyboard mic or aokr type.
-    if (!devices[i].is_for_simple_usage())
+    if (!device.is_for_simple_usage())
       continue;
-    if (devices[i].is_input) {
+    if (device.is_input) {
       // Do not expose the internal front and rear mic to UI.
-      if (has_dual_internal_mic &&
-          audio_handler->IsFrontOrRearMic(devices[i])) {
-        if (devices[i].active)
+      if (has_dual_internal_mic && audio_handler->IsFrontOrRearMic(device)) {
+        if (device.active)
           is_front_or_rear_mic_active = true;
         continue;
       }
-      input_devices_.push_back(devices[i]);
+      input_devices_.push_back(device);
     } else {
-      output_devices_.push_back(devices[i]);
+      output_devices_.push_back(device);
     }
   }
 
@@ -168,14 +129,15 @@ void AudioDetailedView::UpdateScrollableList() {
 
   // Add audio output devices.
   const bool has_output_devices = output_devices_.size() > 0;
-  if (has_output_devices)
-    AddOutputHeader();
-
-  for (size_t i = 0; i < output_devices_.size(); ++i) {
-    HoverHighlightView* container = AddScrollListItem(
-        GetAudioDeviceName(output_devices_[i]), false /* highlight */,
-        output_devices_[i].active); /* checkmark if active */
-    device_map_[container] = output_devices_[i];
+  if (has_output_devices) {
+    AddAudioSubHeader(kSystemMenuAudioOutputIcon,
+                      IDS_ASH_STATUS_TRAY_AUDIO_OUTPUT);
+  }
+
+  for (const auto& device : output_devices_) {
+    HoverHighlightView* container = AddScrollListCheckableItem(
+        gfx::kNoneIcon, GetAudioDeviceName(device), device.active);
+    device_map_[container] = device;
   }
 
   if (has_output_devices) {
@@ -185,14 +147,15 @@ void AudioDetailedView::UpdateScrollableList() {
 
   // Add audio input devices.
   const bool has_input_devices = input_devices_.size() > 0;
-  if (has_input_devices)
-    AddInputHeader();
-
-  for (size_t i = 0; i < input_devices_.size(); ++i) {
-    HoverHighlightView* container = AddScrollListItem(
-        GetAudioDeviceName(input_devices_[i]), false /* highlight */,
-        input_devices_[i].active); /* checkmark if active */
-    device_map_[container] = input_devices_[i];
+  if (has_input_devices) {
+    AddAudioSubHeader(kSystemMenuAudioInputIcon,
+                      IDS_ASH_STATUS_TRAY_AUDIO_INPUT);
+  }
+
+  for (const auto& device : input_devices_) {
+    HoverHighlightView* container = AddScrollListCheckableItem(
+        gfx::kNoneIcon, GetAudioDeviceName(device), device.active);
+    device_map_[container] = device;
   }
 
   scroll_content()->SizeToPreferredSize();