[ash-md] Adjusts layout of lists with sticky header rows to match specs

ash/common/system/chromeos/network/network_list_md.cc

Index: ash/common/system/chromeos/network/network_list_md.cc
diff --git a/ash/common/system/chromeos/network/network_list_md.cc b/ash/common/system/chromeos/network/network_list_md.cc
index 70dc6e779d8b269003f2174e391348309ce3d025..b576bec49687fd9a44c87a660448e70e7e476888 100644
--- a/ash/common/system/chromeos/network/network_list_md.cc
+++ b/ash/common/system/chromeos/network/network_list_md.cc
@@ -33,6 +33,7 @@
 #include "ui/views/border.h"
 #include "ui/views/controls/button/toggle_button.h"
 #include "ui/views/controls/label.h"
+#include "ui/views/controls/separator.h"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/painter.h"
@@ -48,11 +49,6 @@ namespace ash {
 
 namespace {
 
-// TODO(varkha): Merge some of those those constants in tray_constants.h
-const int kSectionHeaderRowSize = 48;
-const int kSectionHeaderRowVerticalInset = 4;
-const int kSectionHeaderRowLeftInset = 18;
-
 bool IsProhibitedByPolicy(const chromeos::NetworkState* network) {
   if (!NetworkTypePattern::WiFi().MatchesType(network->type()))
     return false;
@@ -114,13 +110,6 @@ class NetworkListViewMd::SectionHeaderRowView : public views::View,
   views::View* container() const { return container_; }
   TrayPopupItemStyle* style() const { return style_.get(); }
 
-  // views::View:
-  gfx::Size GetPreferredSize() const override {
-    gfx::Size size = views::View::GetPreferredSize();
-    size.set_height(kSectionHeaderRowSize + kSectionHeaderRowVerticalInset * 2);
-    return size;
-  }
-
   int GetHeightForWidth(int w) const override {
     // Make row height fixed avoiding layout manager adjustments.
     return GetPreferredSize().height();
@@ -139,8 +128,9 @@ class NetworkListViewMd::SectionHeaderRowView : public views::View,
     // https://crbug.com/614453.
     TrayPopupUtils::ConfigureAsStickyHeader(this);
     container_ = new views::View;
-    container_->SetBorder(
-        views::CreateEmptyBorder(0, kSectionHeaderRowLeftInset, 0, 0));
+    container_->SetBorder(views::CreateEmptyBorder(
+        kMenuSeparatorVerticalPadding, kTrayPopupPaddingHorizontal,
+        kMenuSeparatorVerticalPadding, 0));
     views::FillLayout* layout = new views::FillLayout;
     SetLayoutManager(layout);
     AddChildView(container_);
@@ -285,7 +275,9 @@ NetworkListViewMd::NetworkListViewMd(NetworkListDelegate* delegate)
       no_wifi_networks_view_(nullptr),
       no_cellular_networks_view_(nullptr),
       cellular_header_view_(nullptr),
-      wifi_header_view_(nullptr) {
+      wifi_header_view_(nullptr),
+      cellular_separator_view_(nullptr),
+      wifi_separator_view_(nullptr) {
   CHECK(delegate_);
 }
 
@@ -461,11 +453,10 @@ NetworkListViewMd::UpdateNetworkListEntries() {
   const NetworkTypePattern pattern = delegate_->GetNetworkTypePattern();
   if (pattern.MatchesPattern(NetworkTypePattern::Cellular())) {
     if (handler->IsTechnologyAvailable(NetworkTypePattern::Cellular())) {
-      UpdateSectionHeaderRow(
+      index = UpdateSectionHeaderRow(
           NetworkTypePattern::Cellular(),
           handler->IsTechnologyEnabled(NetworkTypePattern::Cellular()), index,
-          &cellular_header_view_);
-      ++index;
+          &cellular_header_view_, &cellular_separator_view_);
     }
 
     // Cellular initializing.
@@ -488,11 +479,10 @@ NetworkListViewMd::UpdateNetworkListEntries() {
   }
 
   if (pattern.MatchesPattern(NetworkTypePattern::WiFi())) {
-    UpdateSectionHeaderRow(
+    index = UpdateSectionHeaderRow(
         NetworkTypePattern::WiFi(),
         handler->IsTechnologyEnabled(NetworkTypePattern::WiFi()), index,
-        &wifi_header_view_);
-    ++index;
+        &wifi_header_view_, &wifi_separator_view_);
 
     // "Wifi Enabled / Disabled".
     int message_id = 0;
@@ -588,10 +578,12 @@ void NetworkListViewMd::UpdateInfoLabel(int message_id,
   *label_ptr = label;
 }
 
-void NetworkListViewMd::UpdateSectionHeaderRow(NetworkTypePattern pattern,
-                                               bool enabled,
-                                               int child_index,
-                                               SectionHeaderRowView** view) {
+int NetworkListViewMd::UpdateSectionHeaderRow(
+    NetworkTypePattern pattern,
+    bool enabled,
+    int child_index,
+    SectionHeaderRowView** view,
+    views::Separator** separator_view) {
   if (!*view) {
     if (pattern.Equals(NetworkTypePattern::Cellular()))
       *view = new CellularHeaderRowView();
@@ -600,9 +592,23 @@ void NetworkListViewMd::UpdateSectionHeaderRow(NetworkTypePattern pattern,
     else
       NOTREACHED();
     (*view)->Init(enabled);
+
+    // Add or remove a separator above the header. The separator should only be
+    // there when the header row is not at the top of the list.
+    if (child_index > 0) {
+      if (!*separator_view)
+        *separator_view = TrayPopupUtils::CreateListHeaderSeparator();
+    } else {
+      if (*separator_view)
+        delete *separator_view;
+      *separator_view = nullptr;

[tdanderson, 2016/11/24 23:37:53]

Instead of deleting, would it be sufficient to just call SetVisible(false)?
(Similarly, call SetVisible(true) in the if-case if the separator has already
been created). When the separator is created it's added as a child view of
|this|, so |this| takes responsibility of the separator's lifetime management I
believe.

[varkha, 2016/11/28 17:14:38]

Done. Simpler indeed.

+    }
   }
   (*view)->SetEnabled(enabled);
-  PlaceViewAtIndex(*view, child_index);
+  if (child_index > 0)
+    PlaceViewAtIndex(*separator_view, child_index++);

[tdanderson, 2016/11/24 23:37:53]

Can you move this to directly after line 600, i.e.,

599 if (!*separator_view)
600   *separator_view = ...
601 PlaceViewAtIndex(*separator_view, child_index++);

then delete line 608?

[varkha, 2016/11/28 17:14:39]

Done.

+  PlaceViewAtIndex(*view, child_index++);
+  return child_index;
 }
 
 void NetworkListViewMd::NetworkIconChanged() {