Refactor AddScrollListItem() in system menu detailed views

ash/system/tray/tray_details_view.h

Index: ash/system/tray/tray_details_view.h
diff --git a/ash/system/tray/tray_details_view.h b/ash/system/tray/tray_details_view.h
index f7f30f4976e6f26fc105b06850097a305aa572e1..8575a1f926e29fab9ccce7d8d2b7e7853605afb8 100644
--- a/ash/system/tray/tray_details_view.h
+++ b/ash/system/tray/tray_details_view.h
@@ -15,6 +15,10 @@
 #include "ui/views/controls/button/button.h"
 #include "ui/views/view.h"
 
+namespace gfx {
+struct VectorIcon;
+}  // namespace gfx
+
 namespace views {
 class BoxLayout;
 class CustomButton;
@@ -27,6 +31,7 @@ namespace test {
 class TrayDetailsViewTest;
 }  // namespace test
 
+class HoverHighlightView;
 class ScrollBorder;
 class SystemTrayItem;
 class TriView;
@@ -47,8 +52,6 @@ class ASH_EXPORT TrayDetailsView : public views::View,
   void ButtonPressed(views::Button* sender, const ui::Event& event) final;
 
   SystemTrayItem* owner() { return owner_; }
-  views::ScrollView* scroller() { return scroller_; }
-  views::View* scroll_content() { return scroll_content_; }
 
  protected:
   // views::View:
@@ -67,6 +70,33 @@ class ASH_EXPORT TrayDetailsView : public views::View,
   // any other view between the list and the footer row at the bottom.
   void CreateScrollableList();
 
+  // Adds a targetable row to |scroll_content_| containing |icon| and |text|.
+  // |gfx::kNoneIcon| can be used if there is no icon.
+  HoverHighlightView* AddScrollListItem(const gfx::VectorIcon& icon,
+                                        const base::string16& text);
+
+  // Adds a targetable row to |scroll_content_| containing |icon|, |text|, and a
+  // checkbox. |checked| determines whether the checkbox is checked or not.
+  // |gfx::kNoneIcon| can be used if there is no icon.
+  HoverHighlightView* AddScrollListCheckableItem(const gfx::VectorIcon& icon,

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

Sorry for the churn here, but how about instead overloading these functions? So
you'd have:

AddScrollListItem(icon, text);
AddScrollListItem(text); // note you currently don't have this.

AddScrollListCheckableItem(icon, text, checked);
AddScrollListCheckableItem(text, checked);

AddScrollListSubHeader(icon, text);
AddScrollListSubHeader(text);

WDYT?

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

I believe preference is not to overload functions.
 (https://google.github.io/styleguide/cppguide.html#Function_Overloading)

Of course, I can overload if you still prefer to, as the OWNER.

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

As discussed offline, I think overloading is appropriate in this instance.

[mohsen, 2017/04/25 21:54:52]

Done.

+                                                 const base::string16& text,
+                                                 bool checked);
+
+  // Convenience function to call AddScrollListCheckableItem() with
+  // |gfx::kNoneIcon|.
+  HoverHighlightView* AddScrollListCheckableItemWithoutIcon(
+      const base::string16& text,
+      bool checked);
+
+  // Adds a sticky sub header to |scroll_content_| containing |icon| and a text
+  // represented by |text_id| resource id. |gfx::kNoneIcon| can be used if there
+  // is no icon.
+  TriView* AddScrollListSubHeader(const gfx::VectorIcon& icon, int text_id);
+
+  // Convenience function to call AddScrollListSubHeader() with
+  // |gfx::kNoneIcon|.

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

nit: I suggest omitting mention of kNoneIcon in the documentation for all the
new functions you're adding here.

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

Removed kNoneIcon from all these functions' comments.

+  TriView* AddScrollListSubHeaderWithoutIcon(int text_id);
+
   // Removes (and destroys) all child views.
   void Reset();
 
@@ -82,6 +112,8 @@ class ASH_EXPORT TrayDetailsView : public views::View,
   views::CustomButton* CreateHelpButton();
 
   TriView* tri_view() { return tri_view_; }
+  views::ScrollView* scroller() const { return scroller_; }
+  views::View* scroll_content() const { return scroll_content_; }
 
  private:
   friend class test::TrayDetailsViewTest;