Refactor AddScrollListItem() in system menu detailed views

ash/system/tray/tray_details_view.h

 // 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.
 
 #ifndef ASH_SYSTEM_TRAY_TRAY_DETAILS_VIEW_H_
 #define ASH_SYSTEM_TRAY_TRAY_DETAILS_VIEW_H_
 
 #include <memory>
 
 #include "ash/ash_export.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/system/tray/view_click_listener.h"
 #include "base/macros.h"
 #include "base/timer/timer.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/view.h"
 
+namespace gfx {
+struct VectorIcon;
+}  // namespace gfx
+
 namespace views {
 class BoxLayout;
 class CustomButton;
 class ProgressBar;
 class ScrollView;
 }  // namespace views
 
 namespace ash {
 namespace test {
 class TrayDetailsViewTest;
 }  // namespace test
 
+class HoverHighlightView;
 class ScrollBorder;
 class SystemTrayItem;
 class TriView;
 
 class ASH_EXPORT TrayDetailsView : public views::View,
                                    public ViewClickListener,
                                    public views::ButtonListener {
  public:
   explicit TrayDetailsView(SystemTrayItem* owner);
   ~TrayDetailsView() override;
 
   // ViewClickListener:
   // Don't override this --- override HandleViewClicked.
   void OnViewClicked(views::View* sender) final;
 
   // views::ButtonListener:
   // Don't override this --- override HandleButtonPressed.
   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:
   void Layout() override;
   int GetHeightForWidth(int width) const override;
 
   // Exposes the layout manager of this view to give control to subclasses.
   views::BoxLayout* box_layout() { return box_layout_; }
 
   // Creates the row containing the back button and title. For material design
   // this appears at the top of the view, for non-material design it appears
   // at the bottom.
   void CreateTitleRow(int string_id);
 
   // Creates a scrollable list. The list has a border at the bottom if there is
   // 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();
 
   // Shows or hides the progress bar below the title row. It occupies the same
   // space as the separator, so when shown the separator is hidden. If
   // |progress_bar_| doesn't already exist it will be created.
   void ShowProgress(double value, bool visible);
 
   // Helper functions which create and return the settings and help buttons,
   // respectively, used in the material design top-most header row. The caller
   // assumes ownership of the returned buttons.
   views::CustomButton* CreateSettingsButton(int setting_accessible_name_id);
   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;
 
   // Overridden to handle clicks on subclass-specific views.
   virtual void HandleViewClicked(views::View* view);
 
   // Overridden to handle button presses on subclass-specific buttons.
   virtual void HandleButtonPressed(views::Button* sender,
                                    const ui::Event& event);
@@ skipping 33 matching lines @@
 
   // Used to delay the transition to the default view.
   base::OneShotTimer transition_delay_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(TrayDetailsView);
 };
 
 }  // namespace ash
 
 #endif  // ASH_SYSTEM_TRAY_TRAY_DETAILS_VIEW_H_