[Chrome OS MD] Update spacing between material design icons in the system tray

ash/common/system/date/date_view.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

[tdanderson, 2016/10/05 21:34:28]

As discussed in person, can you please file a bug along the lines of "take
internal icon padding into account when adjusting tray icon spacing" so that it
can be looked at as part of follow-up work?

[yiyix, 2016/10/05 22:46:47]

Done, I also included a todo in the file.

 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/common/system/date/date_view.h"
 
+#include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_utils.h"
 #include "ash/common/wm_shell.h"
 #include "base/i18n/rtl.h"
 #include "base/i18n/time_formatting.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "grit/ash_strings.h"
 #include "third_party/icu/source/i18n/unicode/datefmt.h"
@@ skipping 17 matching lines @@
 
 // Text color of the vertical clock minutes.
 const SkColor kVerticalClockMinuteColor = SkColorSetRGB(0xBA, 0xBA, 0xBA);
 
 // Padding between the left edge of the shelf and the left edge of the vertical
 // clock.
 const int kVerticalClockLeftPadding = 9;
 
 // Offset used to bring the minutes line closer to the hours line in the
 // vertical clock.
-const int kVerticalClockMinutesTopOffset = -4;
+const int kVerticalClockMinutesTopOffset = -2;

[tdanderson, 2016/10/05 21:34:29]

This constant is used on line 346 which can be executed in non-MD. Does changing
this constant to always be -4 fix a bug in non-MD? If not, you should keep this
as -4 for non-MD and only use the -2 value for MD.

[yiyix, 2016/10/05 22:46:47]

You are right. I forget about the non-md path.

+
+// Padding used to draw the tray background around the clock.
+const int kClockOnTrayBackgroundPadding = 8;

[tdanderson, 2016/10/05 21:34:28]

Since this is only used for a vertical shelf, can you rename the variable to
something like kClockVerticalShelfPadding (and update the comment accordingly)

[yiyix, 2016/10/05 22:46:47]

It is used for both horizontal and vertical aligned shelf, in line 304 and line
323, respectively. 

 
 base::string16 FormatDate(const base::Time& time) {
   icu::UnicodeString date_string;
   std::unique_ptr<icu::DateFormat> formatter(
       icu::DateFormat::createDateInstance(icu::DateFormat::kMedium));
   formatter->format(static_cast<UDate>(time.ToDoubleT() * 1000), date_string);
   return base::string16(date_string.getBuffer(),
                         static_cast<size_t>(date_string.length()));
 }
 
@@ skipping 225 matching lines @@
 
 void TimeView::UpdateClockLayout(TrayDate::ClockLayout clock_layout) {
   SetBorderFromLayout(clock_layout);
   if (clock_layout == TrayDate::HORIZONTAL_CLOCK) {
     RemoveChildView(vertical_label_hours_.get());
     RemoveChildView(vertical_label_minutes_.get());
     SetLayoutManager(
         new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0));
     AddChildView(horizontal_label_.get());
   } else {
+    bool is_material_design = MaterialDesignController::IsShelfMaterial();

[tdanderson, 2016/10/05 21:34:28]

nit: const

[yiyix, 2016/10/05 22:46:47]

Done.

     RemoveChildView(horizontal_label_.get());
     views::GridLayout* layout = new views::GridLayout(this);
     SetLayoutManager(layout);
     const int kColumnId = 0;
     views::ColumnSet* columns = layout->AddColumnSet(kColumnId);
     columns->AddPaddingColumn(0, kVerticalClockLeftPadding);
     columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
                        0, views::GridLayout::USE_PREF, 0, 0);
-    layout->AddPaddingRow(0, kTrayLabelItemVerticalPaddingVerticalAlignment);
+    layout->AddPaddingRow(
+        0, is_material_design ? kClockOnTrayBackgroundPadding
+                              : kTrayLabelItemVerticalPaddingVerticalAlignment);
     layout->StartRow(0, kColumnId);
     layout->AddView(vertical_label_hours_.get());
     layout->StartRow(0, kColumnId);
     layout->AddView(vertical_label_minutes_.get());
-    layout->AddPaddingRow(0, kTrayLabelItemVerticalPaddingVerticalAlignment);
+    layout->AddPaddingRow(0,
+                          is_material_design
+                              ? GetTrayConstant(TRAY_IMAGE_ITEM_PADDING) +
+                                    kVerticalClockMinutesTopOffset
+                              : kTrayLabelItemVerticalPaddingVerticalAlignment);
   }
   Layout();
 }
 
 void TimeView::SetBorderFromLayout(TrayDate::ClockLayout clock_layout) {
-  if (clock_layout == TrayDate::HORIZONTAL_CLOCK)
-    SetBorder(views::Border::CreateEmptyBorder(
-        0, kTrayLabelItemHorizontalPaddingBottomAlignment, 0,
-        kTrayLabelItemHorizontalPaddingBottomAlignment));
-  else
+  if (clock_layout == TrayDate::HORIZONTAL_CLOCK) {
+    bool is_material_design = MaterialDesignController::IsShelfMaterial();
+    const int time_view_left_padding =
+        is_material_design ? kClockOnTrayBackgroundPadding
+                           : kTrayLabelItemHorizontalPaddingBottomAlignment;
+    const int time_view_right_padding =
+        is_material_design ? GetTrayConstant(TRAY_IMAGE_ITEM_PADDING)
+                           : kTrayLabelItemHorizontalPaddingBottomAlignment;
+    SetBorder(views::Border::CreateEmptyBorder(0, time_view_left_padding, 0,
+                                               time_view_right_padding));
+  } else {

[tdanderson, 2016/10/05 21:34:28]

Thanks for adding the {} !

[yiyix, 2016/10/05 22:46:47]

:)

     SetBorder(views::Border::NullBorder());
+  }
 }
 
 void TimeView::SetupLabels() {
   horizontal_label_.reset(new views::Label());
   SetupLabel(horizontal_label_.get());
   vertical_label_hours_.reset(new views::Label());
   SetupLabel(vertical_label_hours_.get());
   vertical_label_minutes_.reset(new views::Label());
   SetupLabel(vertical_label_minutes_.get());
   // TODO(estade): this should use the NativeTheme's secondary text color.
   vertical_label_minutes_->SetEnabledColor(kVerticalClockMinuteColor);
   // Pull the minutes up closer to the hours by using a negative top border.
   vertical_label_minutes_->SetBorder(views::Border::CreateEmptyBorder(
       kVerticalClockMinutesTopOffset, 0, 0, 0));
 }
 
 void TimeView::SetupLabel(views::Label* label) {
   label->set_owned_by_client();
   SetupLabelForTray(label);
   label->SetElideBehavior(gfx::NO_ELIDE);
 }
 
 }  // namespace tray
 }  // namespace ash