MenuButton: support Mac look & feel

ui/views/controls/button/menu_button.cc

Index: ui/views/controls/button/menu_button.cc
diff --git a/ui/views/controls/button/menu_button.cc b/ui/views/controls/button/menu_button.cc
index 4d1a1b9728157e8581a4ec725a94de28183a7715..815306fb1fcefe670031fe78dd99afa9730ce136 100644
--- a/ui/views/controls/button/menu_button.cc
+++ b/ui/views/controls/button/menu_button.cc
@@ -21,8 +21,10 @@
 #include "ui/views/animation/ink_drop_delegate.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/menu_button_listener.h"
+#include "ui/views/controls/focusable_border.h"

[tapted, 2016/04/26 05:05:57]

is this still needed?

[Elly Fong-Jones, 2016/04/26 17:41:25]

It seems to be needed so that the destructor can delete a
unique_ptr<FocusableBorder>.

[tapted, 2016/04/27 03:25:11]

Ah, that's subtle. I think we could have PlatformStyle::CreateMenuButtonBorder()
return a vanilla views::Border, but that might just require a border.h include
instead. So I think what you have is fine, but could go either way.

 #include "ui/views/mouse_constants.h"
 #include "ui/views/resources/grit/views_resources.h"
+#include "ui/views/style/platform_style.h"
 #include "ui/views/widget/root_view.h"
 #include "ui/views/widget/widget.h"
 
@@ -73,15 +75,19 @@ MenuButton::MenuButton(const base::string16& text,
       menu_offset_(kDefaultMenuOffsetX, kDefaultMenuOffsetY),
       listener_(menu_button_listener),
       show_menu_marker_(show_menu_marker),
-      menu_marker_(ui::ResourceBundle::GetSharedInstance()
-                       .GetImageNamed(IDR_MENU_DROPARROW)
-                       .ToImageSkia()),
       destroyed_flag_(nullptr),
       pressed_lock_count_(0),
       increment_pressed_lock_called_(nullptr),
       should_disable_after_press_(false),
       weak_factory_(this) {
   SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  SetBorder(PlatformStyle::CreateMenuButtonBorder());
+  if (show_menu_marker_) {
+    enabled_marker_image_ = PlatformStyle::CreateMenuButtonArrow(true);
+    disabled_marker_image_ = PlatformStyle::CreateMenuButtonArrow(false);
+    set_background(PlatformStyle::CreateMenuButtonBackground(GetShoulderWidth())
+                       .release());
+  }
 }
 
 MenuButton::~MenuButton() {
@@ -196,13 +202,17 @@ void MenuButton::OnPaint(gfx::Canvas* canvas) {
 //
 ////////////////////////////////////////////////////////////////////////////////
 
+int MenuButton::GetShoulderWidth() const {
+  const int kPadding = 7;
+  int marker_width = enabled() ? enabled_marker_image_.size().width()

[tapted, 2016/04/26 05:05:57]

nit: remove ".size()"

I think we can also do


if (!show_menu_marker_)
  return 0;

DCHECK_EQ(enabled_marker_image_.width(), disabled_marker_image_.width());
return enabled_marker_image_.width() + 2 * kPadding;



The early return means we can simplify GetPreferredSize() and
GetChildAreaBounds(), and the DCHECK simplifies things -- if it wasn't true,
we'd need to trigger a re-layout when the enable state changes, which I don't
think we do


[edit: that DCHECK will be unnecessary when we switch to just having one
ImageSkia]

[Elly Fong-Jones, 2016/04/26 17:41:25]

Done.

+                               : disabled_marker_image_.size().width();
+  return marker_width + 2 * kPadding;
+}
+
 gfx::Size MenuButton::GetPreferredSize() const {
   gfx::Size prefsize = LabelButton::GetPreferredSize();
-  if (show_menu_marker_) {
-    prefsize.Enlarge(menu_marker_->width() + kMenuMarkerPaddingLeft +
-                         kMenuMarkerPaddingRight,
-                     0);
-  }
+  if (show_menu_marker_)
+    prefsize.Enlarge(GetShoulderWidth(), 0);
   return prefsize;
 }
 
@@ -316,22 +326,20 @@ void MenuButton::PaintMenuMarker(gfx::Canvas* canvas) {
 
   // Using the Views mirroring infrastructure incorrectly flips icon content.
   // Instead, manually mirror the position of the down arrow.
-  gfx::Rect arrow_bounds(width() - insets.right() -
-                         menu_marker_->width() - kMenuMarkerPaddingRight,
-                         height() / 2 - menu_marker_->height() / 2,
-                         menu_marker_->width(),
-                         menu_marker_->height());
+  gfx::Rect arrow_bounds(width() - insets.right() - GetShoulderWidth(), 0,
+                         GetShoulderWidth(), height());
+  gfx::ImageSkia marker =
+      enabled() ? enabled_marker_image_ : disabled_marker_image_;
+  arrow_bounds.ClampToCenteredSize(marker.size());
   arrow_bounds.set_x(GetMirroredXForRect(arrow_bounds));
-  canvas->DrawImageInt(*menu_marker_, arrow_bounds.x(), arrow_bounds.y());
+  canvas->DrawImageInt(marker, arrow_bounds.x(), arrow_bounds.y());
 }
 
 gfx::Rect MenuButton::GetChildAreaBounds() {
   gfx::Size s = size();
 
-  if (show_menu_marker_) {
-    s.set_width(s.width() - menu_marker_->width() - kMenuMarkerPaddingLeft -
-                kMenuMarkerPaddingRight);
-  }
+  if (show_menu_marker_)

[tapted, 2016/04/26 05:05:57]

not needed with the GetShoulderWidth() tweak. In fact the whole method can just
be

  return gfx::Rect(0, 0, width() - GetShoulderWidth(), height());

[Elly Fong-Jones, 2016/04/26 17:41:25]

Done.

+    s.set_width(s.width() - GetShoulderWidth());
 
   return gfx::Rect(s);
 }