More closely align palette to spec.

ash/common/system/chromeos/palette/palette_tray.cc

Index: ash/common/system/chromeos/palette/palette_tray.cc
diff --git a/ash/common/system/chromeos/palette/palette_tray.cc b/ash/common/system/chromeos/palette/palette_tray.cc
index 9d01c2b0733a7092c95ad8f23305aa4f53e50c6a..2a3e401191e69b61918b05bfdc13f38a9b6a34f6 100644
--- a/ash/common/system/chromeos/palette/palette_tray.cc
+++ b/ash/common/system/chromeos/palette/palette_tray.cc
@@ -22,7 +22,9 @@
 #include "grit/ash_strings.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/vector_icons_public.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/separator.h"
@@ -41,20 +43,24 @@ const int kVerticalShelfHorizontalPadding = 2;
 const int kVerticalShelfVerticalPadding = 5;
 
 // Width of the palette itself (dp).
-const int kPaletteWidth = 360;
+const int kPaletteWidth = 332;
 
 // Size of icon in the shelf (dp).
 const int kShelfIconSize = 18;
 
+// Margins between the title view and the edges around it (dp).
+const int kPaddingBetweenTitleAndTopEdge = 4;
+const int kPaddingBetweenTitleAndLeftEdge = 12;
+const int kPaddingBetweenTitleAndSeparator = 3;
+
+// Size of the header icons (dp).
+const int kIconSize = 20;
+
 // Creates a separator.
 views::Separator* CreateSeparator(views::Separator::Orientation orientation) {
-  const int kSeparatorInset = 10;
-
   views::Separator* separator =
       new views::Separator(views::Separator::HORIZONTAL);
   separator->SetColor(ash::kBorderDarkColor);
-  separator->SetBorder(
-      views::Border::CreateEmptyBorder(kSeparatorInset, 0, kSeparatorInset, 0));
   return separator;
 }
 
@@ -66,8 +72,6 @@ class TitleView : public views::View, public views::ButtonListener {
     auto* box_layout =
         new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
     SetLayoutManager(box_layout);
-    SetBorder(views::Border::CreateEmptyBorder(
-        0, ash::kTrayPopupPaddingHorizontal, 0, 0));
 
     views::Label* text_label =
         new views::Label(l10n_util::GetStringUTF16(IDS_ASH_PALETTE_TITLE));
@@ -76,16 +80,20 @@ class TitleView : public views::View, public views::ButtonListener {
     AddChildView(text_label);
     box_layout->SetFlexForView(text_label, 1);
 
-    help_button_ = new ash::TrayPopupHeaderButton(this, IDR_AURA_UBER_TRAY_HELP,
+    gfx::ImageSkia settings_icon = CreateVectorIcon(
+        gfx::VectorIconId::SETTINGS, kIconSize, gfx::kChromeIconGrey);
+    // TODO: Add HELP icon

[stevenjb, 2016/08/24 15:58:57]

TODO(jdufault)

[jdufault, 2016/08/24 18:48:06]

Oops - I forgot to delete that before submitting :/

+    gfx::ImageSkia help_icon = CreateVectorIcon(
+        gfx::VectorIconId::HELP, kIconSize, gfx::kChromeIconGrey);
+
+    help_button_ = new ash::TrayPopupHeaderButton(this, help_icon,
                                                   IDS_ASH_STATUS_TRAY_HELP);
     help_button_->SetTooltipText(
-        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SHUTDOWN));
+        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_HELP));
     AddChildView(help_button_);
 
-    AddChildView(CreateSeparator(views::Separator::VERTICAL));
-
     settings_button_ = new ash::TrayPopupHeaderButton(
-        this, IDR_AURA_UBER_TRAY_SETTINGS, IDS_ASH_STATUS_TRAY_SETTINGS);
+        this, settings_icon, IDS_ASH_STATUS_TRAY_SETTINGS);
     settings_button_->SetTooltipText(
         l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SETTINGS));
     AddChildView(settings_button_);
@@ -170,11 +178,19 @@ bool PaletteTray::OpenBubble() {
       views::TrayBubbleView::Create(tray_container(), this, &init_params);
   bubble_view->SetArrowPaintType(views::BubbleBorder::PAINT_NONE);
   bubble_view->UseCompactMargins();
-  bubble_view->set_margins(gfx::Insets(bubble_view->margins().top(), 0, 0, 0));
+  bubble_view->set_margins(gfx::Insets(0, 0, 0, 0));
 
   // Add child views.
-  bubble_view->AddChildView(new TitleView(this));
-  bubble_view->AddChildView(CreateSeparator(views::Separator::HORIZONTAL));
+  auto* title_view = new TitleView(this);
+  title_view->SetBorder(views::Border::CreateEmptyBorder(gfx::Insets(
+      kPaddingBetweenTitleAndTopEdge, kPaddingBetweenTitleAndLeftEdge,
+      kPaddingBetweenTitleAndSeparator, 0)));
+  bubble_view->AddChildView(title_view);
+
+  auto* separator = CreateSeparator(views::Separator::HORIZONTAL);

[stevenjb, 2016/08/24 15:58:57]

We generally avoid using auto* like this. It's allowed in line 184 since the
type is already there (but doesn't save much and is a little inconsistent with
other places, e.g. line 177), but here it is less clear what exactly |separator|
is. i.e. auto is great for unwieldy iterators, but less ideal for a simple
pointer to a class.

There are more guidelines in the Google Style Guide:
https://google.github.io/styleguide/cppguide.html#auto

[jdufault, 2016/08/24 18:48:06]

Done.

+  separator->SetBorder(
+      views::Border::CreateEmptyBorder(gfx::Insets(0, 0, 4, 0)));
+  bubble_view->AddChildView(separator);
   AddToolsToView(bubble_view);
 
   // Show the bubble.
@@ -187,21 +203,8 @@ bool PaletteTray::OpenBubble() {
 
 void PaletteTray::AddToolsToView(views::View* host) {
   std::vector<PaletteToolView> views = palette_tool_manager_->CreateViews();
-
-  // There may not be any registered tools.
-  if (!views.size())
-    return;
-
-  PaletteGroup group = views[0].group;
-  for (const PaletteToolView& view : views) {
-    // If the group changes, add a separator.
-    if (group != view.group) {
-      group = view.group;
-      host->AddChildView(CreateSeparator(views::Separator::HORIZONTAL));
-    }
-
+  for (const PaletteToolView& view : views)
     host->AddChildView(view.view);
-  }
 }
 
 void PaletteTray::SessionStateChanged(
@@ -244,10 +247,18 @@ gfx::Rect PaletteTray::GetAnchorRect(
 
   // Move the palette to the left so the right edge of the palette aligns with
   // the right edge of the tray button.
-  if (IsHorizontalAlignment(shelf_alignment()))
-    r.Offset(-r.width() + tray_container()->width(), 0);
-  else
+  if (IsHorizontalAlignment(shelf_alignment())) {
+    // TODO(jdufault): Figure out a more robust adjustment method that does not
+    // break in md-shelf.
+    int icon_size = tray_container()->width();
+    if (tray_container()->border())
+      icon_size -= tray_container()->border()->GetInsets().width();
+
+    r.Offset(-r.width() + icon_size, 0);
+  } else {
+    // Vertical layout doesn't need the border adjustment that horizontal needs.
     r.Offset(0, -r.height() + tray_container()->height());
+  }
 
   return r;
 }