Refactor AddScrollListItem() in system menu detailed views

ash/system/tray/tray_popup_utils.h

 // Copyright 2016 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_POPUP_UTILS_H_
 #define ASH_SYSTEM_TRAY_TRAY_POPUP_UTILS_H_
 
 #include <memory>
 
 #include "ash/login_status.h"
@@ skipping 36 matching lines @@
   //
   // The START and END containers have a fixed minimum width but can grow into
   // the CENTER container if space is required and available.
   //
   // The CENTER container has a flexible width.
   static TriView* CreateDefaultRowView();
 
   // Creates a container view to be used by system menu sub-section header rows.
   // The caller takes over ownership of the created view.
   //
-  // The returned view consists of 2 regions: CENTER, and END having the same
-  // properties as when using |CreateMultiTargetRowView|. The START container is
-  // hidden.
-  // The END container has a fixed minimum width but can grow into the CENTER
-  // container if space is required and available. The CENTER container has a
-  // flexible width.
-  static TriView* CreateSubHeaderRowView();
+  // The returned view contains at least CENTER and END regions having the same
+  // properties as when using |CreateMultiTargetRowView|. |hide_start|
+  // determines whether the START region should be visible or not. If START is
+  // not visible, padding would be increased on that side.

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

I'm not sure about the last sentence here. If START is not visible, don't you
have CENTER beginning left-aligned in the container?

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

What I meant is that when START is not visible, we add extra padding to the left
of the container. Modified the comment to hopefully make it clearer.

+  //
+  // The START (if visible) and END containers have a fixed minimum width but
+  // can grow into the CENTER container if space is required and available. The
+  // CENTER container has a flexible width.
+  static TriView* CreateSubHeaderRowView(bool hide_start);

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

optional: I don't think this is called out explicitly in the style guide, and
maybe this is purely personal preference, but I feel that naming variables in a
'positive' rather than a 'negative' way usually makes things more readable. So
here, consider calling the parameter something like |start_visible|, and
swapping all call sites from true to false and vice-versa.

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

Makes sense. Done.

 
   // Creates a view containing only a label (corresponding to |message_id|),
   // which is to be inserted as a non-targetable row within a system menu
   // detailed view (e.g., the "Scanning for devices..." message that can appear
   // at the top of the Bluetooth detailed view). The caller takes over ownership
   // of the created view.
   static views::View* CreateInfoLabelRowView(int message_id);
 
   // Creates a container view to be used by system menu rows that want to embed
   // a targetable area within one (or more) of the containers OR by any row
@@ skipping 155 matching lines @@
   // |ink_drop_style|.
   static gfx::Rect GetInkDropBounds(TrayPopupInkDropStyle ink_drop_style,
                                     const views::View* host);
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(TrayPopupUtils);
 };
 
 }  // namespace ash
 
 #endif  // ASH_SYSTEM_TRAY_TRAY_POPUP_UTILS_H_