More closely align palette to spec.

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

 // 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.
 
 #include "ash/common/system/chromeos/palette/palette_tray.h"
 
 #include "ash/common/shelf/shelf_constants.h"
 #include "ash/common/shelf/wm_shelf.h"
 #include "ash/common/shelf/wm_shelf_util.h"
 #include "ash/common/shell_window_ids.h"
 #include "ash/common/system/chromeos/palette/palette_tool_manager.h"
 #include "ash/common/system/chromeos/palette/palette_utils.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/tray_bubble_wrapper.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_popup_header_button.h"
 #include "ash/common/wm_lookup.h"
 #include "ash/common/wm_root_window_controller.h"
 #include "ash/common/wm_shell.h"
 #include "ash/common/wm_window.h"
 #include "grit/ash_resources.h"
 #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"
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 
 namespace ash {
 
 namespace {
 
 // Predefined padding for the icon used in this tray. These are to be set to the
 // border of the icon, depending on the current |shelf_alignment()|.
 const int kHorizontalShelfHorizontalPadding = 8;
 const int kHorizontalShelfVerticalPadding = 4;
 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;
 }
 
 class TitleView : public views::View, public views::ButtonListener {
  public:
   explicit TitleView(PaletteTray* palette_tray) : palette_tray_(palette_tray) {
     auto& rb = ui::ResourceBundle::GetSharedInstance();
 
     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));
     text_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
     text_label->SetFontList(rb.GetFontList(ui::ResourceBundle::BoldFont));
     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_);
   }
 
   ~TitleView() override {}
 
  private:
   // views::ButtonListener:
   void ButtonPressed(views::Button* sender, const ui::Event& event) override {
@@ skipping 64 matching lines @@
   init_params.can_activate = true;
   init_params.close_on_deactivate = true;
 
   DCHECK(tray_container());
 
   // Create view, customize it.
   views::TrayBubbleView* bubble_view =
       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.
   bubble_.reset(new ash::TrayBubbleWrapper(this, bubble_view));
 
   SetDrawBackgroundAsActive(true);
 
   return true;
 }
 
 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(
     SessionStateDelegate::SessionState state) {
   UpdateIconVisibility();
 }
 
 void PaletteTray::ClickedOutsideBubble() {
   HidePalette();
 }
@@ skipping 22 matching lines @@
 
 gfx::Rect PaletteTray::GetAnchorRect(
     views::Widget* anchor_widget,
     views::TrayBubbleView::AnchorType anchor_type,
     views::TrayBubbleView::AnchorAlignment anchor_alignment) const {
   gfx::Rect r =
       GetBubbleAnchorRect(anchor_widget, anchor_type, anchor_alignment);
 
   // 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;
 }
 
 void PaletteTray::OnBeforeBubbleWidgetInit(
     views::Widget* anchor_widget,
     views::Widget* bubble_widget,
     views::Widget::InitParams* params) const {
   // Place the bubble in the same root window as |anchor_widget|.
   WmLookup::Get()
@@ skipping 58 matching lines @@
       WmShell::Get()->GetSessionStateDelegate();
 
   SetVisible(!session_state_delegate->IsScreenLocked() &&
              session_state_delegate->GetSessionState() ==
                  SessionStateDelegate::SESSION_STATE_ACTIVE &&
              WmShell::Get()->system_tray_delegate()->GetUserLoginStatus() !=
                  LoginStatus::KIOSK_APP);
 }
 
 }  // namespace ash