MenuButton: support Mac look & feel

ui/views/controls/button/menu_button_unittest.cc

Index: ui/views/controls/button/menu_button_unittest.cc
diff --git a/ui/views/controls/button/menu_button_unittest.cc b/ui/views/controls/button/menu_button_unittest.cc
index 86ee12b061c249bf12fb4b4a346411c4a8e46755..fc221eac1ae927de55db03eb07f508e684110693 100644
--- a/ui/views/controls/button/menu_button_unittest.cc
+++ b/ui/views/controls/button/menu_button_unittest.cc
@@ -13,7 +13,9 @@
 #include "ui/events/test/event_generator.h"
 #include "ui/views/animation/test/test_ink_drop_delegate.h"
 #include "ui/views/controls/button/menu_button_listener.h"
+#include "ui/views/controls/focusable_border.h"
 #include "ui/views/drag_controller.h"
+#include "ui/views/style/platform_style.h"
 #include "ui/views/test/views_test_base.h"
 
 #if defined(USE_AURA)
@@ -32,19 +34,32 @@ namespace test {
 // A MenuButton subclass that provides access to some MenuButton internals.
 class TestMenuButton : public MenuButton {
  public:
-  explicit TestMenuButton(MenuButtonListener* menu_button_listener)
+  TestMenuButton(MenuButtonListener* menu_button_listener,
+                 bool use_background,

[tapted, 2016/05/09 08:01:35]

I don't think we need to pass in these arguments -- just add setters for
use_background_ and use_border_. Then we avoid a lot of the default arguments
which can be annoying to refactor around (like, what if we add a new feature
that requires a third?)

+                 bool use_border)
       : MenuButton(base::string16(ASCIIToUTF16("button")),
                    menu_button_listener,
-                   false) {}
+                   false),
+        use_background_(use_background),
+        use_border_(use_border) {}
 
   ~TestMenuButton() override {}
 
+  // Overrides for MenuButton customization methods.

[tapted, 2016/05/09 08:01:35]

nit: just
// MenuButton:

+  bool ShouldUsePlatformStyleBackground() const override {
+    return use_background_;
+  }
+
+  bool ShouldUsePlatformStyleBorder() const override { return use_border_; }
+
   // Accessors to protected MenuButton methods.
   void set_ink_drop_delegate(InkDropDelegate* ink_drop_delegate) {
     MenuButton::set_ink_drop_delegate(ink_drop_delegate);
   }
 
  private:
+  bool use_background_;
+  bool use_border_;
   DISALLOW_COPY_AND_ASSIGN(TestMenuButton);

[tapted, 2016/05/09 08:01:35]

nit: blank line before

 };
 
@@ -67,18 +82,25 @@ class MenuButtonTest : public ViewsTestBase {
 
  protected:
   // Creates a MenuButton with no button listener.
-  void CreateMenuButtonWithNoListener() { CreateMenuButton(nullptr); }
+  void CreateMenuButtonWithNoListener(bool use_background = false,
+                                      bool use_border = false) {
+    CreateMenuButton(nullptr, use_background, use_border);
+  }
 
   // Creates a MenuButton with a MenuButtonListener. In this case, when the
   // MenuButton is pushed, it notifies the MenuButtonListener to open a
   // drop-down menu.
   void CreateMenuButtonWithMenuButtonListener(
-      MenuButtonListener* menu_button_listener) {
-    CreateMenuButton(menu_button_listener);
+      MenuButtonListener* menu_button_listener,
+      bool use_background = false,
+      bool use_border = false) {
+    CreateMenuButton(menu_button_listener, use_background, use_border);
   }
 
  private:
-  void CreateMenuButton(MenuButtonListener* menu_button_listener) {
+  void CreateMenuButton(MenuButtonListener* menu_button_listener,
+                        bool use_background,
+                        bool use_border) {
     CreateWidget();
     generator_.reset(new ui::test::EventGenerator(GetContext(),
         widget_->GetNativeWindow()));
@@ -86,7 +108,8 @@ class MenuButtonTest : public ViewsTestBase {
     // are about to create initializes its hover state in a consistent manner.
     generator_->set_current_location(gfx::Point(10, 10));
 
-    button_ = new TestMenuButton(menu_button_listener);
+    button_ =
+        new TestMenuButton(menu_button_listener, use_background, use_border);
     button_->SetBoundsRect(gfx::Rect(0, 0, 200, 20));
     widget_->SetContentsView(button_);
 
@@ -609,5 +632,17 @@ TEST_F(MenuButtonTest, TouchFeedbackDuringTapCancel) {
 
 #endif  // !defined(OS_MACOSX) || defined(USE_AURA)
 
+TEST_F(MenuButtonTest, UseBorderDoesApplyBorder) {

[tapted, 2016/05/09 08:01:35]

tests should have a comment describing why they exist

+  CreateMenuButtonWithNoListener(false, true);
+  if (PlatformStyle::CreateMenuButtonBorder())

[tapted, 2016/05/09 08:01:35]

I think it would be better to #if defined(platform) for this -- makes it more
obvious that the test is testing specific rather than inferred behavior.

+    EXPECT_TRUE(button()->border());
+}

[tapted, 2016/05/09 08:01:35]

we also need to test something that calls SetBorder *before* the button is added
to the Widget -- this is the current bug with the Chrome menu button.

+
+TEST_F(MenuButtonTest, UseBackgroundDoesApplyBackground) {
+  CreateMenuButtonWithNoListener(true, false);
+  if (PlatformStyle::CreateMenuButtonBackground(10))
+    EXPECT_TRUE(button()->background());
+}
+
 }  // namespace views
 }  // namespace test