Update shelf spacing in Chrome OS according to the MD specs

ash/system/status_area_widget_delegate.cc

 // 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.
-

[tdanderson, 2016/06/03 19:25:23]

nit: replace this blank line

[yiyix, 2016/06/10 19:26:05]

Done.

 #include "ash/system/status_area_widget_delegate.h"
 
 #include "ash/ash_export.h"
 #include "ash/ash_switches.h"
 #include "ash/focus_cycler.h"
 #include "ash/shelf/shelf_util.h"
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
 #include "ash/system/tray/tray_constants.h"
 #include "ash/wm/common/shelf/wm_shelf_util.h"
@@ skipping 92 matching lines @@
   UpdateLayout();
 }
 
 void StatusAreaWidgetDelegate::UpdateLayout() {
   // Use a grid layout so that the trays can be centered in each cell, and
   // so that the widget gets laid out correctly when tray sizes change.
   views::GridLayout* layout = new views::GridLayout(this);
   SetLayoutManager(layout);
 
   views::ColumnSet* columns = layout->AddColumnSet(0);
+  bool is_first_visible_child = true;

[bruthig, 2016/06/03 20:18:17]

I think keeping this variable in the tightest scope makes it a lot easier to
understand.  I understand there was a similarly named variable in the 'else'
branch below but pulling in to the outer scope here makes me see if it's used
beyond the if/else block.

[yiyix, 2016/06/10 19:26:04]

Done.

   if (wm::IsHorizontalAlignment(alignment_)) {
-    bool is_first_visible_child = true;
-    for (int c = 0; c < child_count(); ++c) {
+    for (int c = child_count() - 1; c >= 0; --c) {
       views::View* child = child_at(c);
       if (!child->visible())
         continue;
-      if (!is_first_visible_child)
-        columns->AddPaddingColumn(0, kTraySpacing);
+      if (!is_first_visible_child) {
+        // If the overview Button is visible, then do not add the additional

[bruthig, 2016/06/03 20:18:17]

I agree this needs a comment, but the 'why' is more valuable than the 'what'. 
Suggestion:

'No padding is added between the OverviewButton and the StatusTray because the
StatusTray includes an extra wide inset on the right side ..."

(This may become irrelevant with the new approach).   




     

[yiyix, 2016/06/10 19:26:05]

I will add some explanation for the new approach too.

+        // empty padding row to the left side to the overview button; otherwise,
+        // add the empty padding to separate items.
+        if (!((child_at(kOverviewButtonIndex)->visible()) &&

[bruthig, 2016/06/03 20:18:17]

I think you have an set of extra brackets around
'child_at(kOverviewButtonIndex)->visible()'

[yiyix, 2016/06/10 19:26:05]

You are right! I didn't notice it!

+              c == kOverviewButtonIndex))
+          columns->AddPaddingColumn(0, GetTrayConstant(TRAY_SPACING));

[tdanderson, 2016/06/03 19:25:23]

nit: use {}

[yiyix, 2016/06/10 19:26:04]

Done.

+      }
       is_first_visible_child = false;
       columns->AddColumn(views::GridLayout::CENTER, views::GridLayout::FILL,
                          0, /* resize percent */
                          views::GridLayout::USE_PREF, 0, 0);
     }
     layout->StartRow(0, 0);
     for (int c = child_count() - 1; c >= 0; --c) {
       views::View* child = child_at(c);
       if (child->visible())
         layout->AddView(child);
     }
+

[tdanderson, 2016/06/03 19:25:23]

nit: remove blank line

[yiyix, 2016/06/10 19:26:04]

Done.

   } else {
     columns->AddColumn(views::GridLayout::FILL, views::GridLayout::CENTER,
                        0, /* resize percent */
                        views::GridLayout::USE_PREF, 0, 0);
     bool is_first_visible_child = true;
     for (int c = child_count() - 1; c >= 0; --c) {
       views::View* child = child_at(c);
       if (!child->visible())
         continue;
-      if (!is_first_visible_child)
-        layout->AddPaddingRow(0, kTraySpacing);
+      if (!is_first_visible_child) {
+        if (!((child_at(kOverviewButtonIndex)->visible()) &&
+              c == kOverviewButtonIndex))
+          layout->AddPaddingRow(0, GetTrayConstant(TRAY_SPACING));

[tdanderson, 2016/06/03 19:25:24]

nit: use {} since the if + boolean condition takes up more than one line.

+      }
       is_first_visible_child = false;
       layout->StartRow(0, 0);
       layout->AddView(child);
     }
   }
 
   layer()->GetAnimator()->StopAnimating();
   StatusAreaWidgetDelegateAnimationSettings settings(layer());
 
   Layout();
   UpdateWidgetSize();
 }
 
 void StatusAreaWidgetDelegate::ChildPreferredSizeChanged(View* child) {
   // Need to resize the window when trays or items are added/removed.
   StatusAreaWidgetDelegateAnimationSettings settings(layer());
   UpdateWidgetSize();
 }
 
 void StatusAreaWidgetDelegate::ChildVisibilityChanged(View* child) {
   UpdateLayout();
 }
 
 void StatusAreaWidgetDelegate::UpdateWidgetSize() {
   if (GetWidget())
     GetWidget()->SetSize(GetPreferredSize());
 }
 
 }  // namespace ash