MenuButton: support Mac look & feel

ui/views/controls/button/menu_button_unittest.cc

 // Copyright 2014 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 "ui/views/controls/button/menu_button.h"
 
 #include <memory>
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "ui/base/dragdrop/drag_drop_types.h"
 #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)
 #include "ui/events/event.h"
 #include "ui/events/event_handler.h"
 #include "ui/wm/public/drag_drop_client.h"
 #endif
 
 using base::ASCIIToUTF16;
 
 namespace views {
 class InkDropDelegate;
 
 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

 };
 
 class MenuButtonTest : public ViewsTestBase {
  public:
   MenuButtonTest() : widget_(nullptr), button_(nullptr) {}
   ~MenuButtonTest() override {}
 
   void TearDown() override {
     generator_.reset();
     if (widget_ && !widget_->IsClosed())
       widget_->Close();
 
     ViewsTestBase::TearDown();
   }
 
   Widget* widget() { return widget_; }
   TestMenuButton* button() { return button_; }
   ui::test::EventGenerator* generator() { return generator_.get(); }
 
  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()));
     // Set initial mouse location in a consistent way so that the menu button we
     // 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_);
 
     widget_->Show();
   }
 
   void CreateWidget() {
     DCHECK(!widget_);
 
     widget_ = new Widget;
@@ skipping 502 matching lines @@
   EXPECT_EQ(Button::STATE_HOVERED, button()->state());
 
   generator()->MoveTouch(gfx::Point(10, 30));
   generator()->ReleaseTouch();
   EXPECT_EQ(Button::STATE_NORMAL, button()->state());
   EXPECT_EQ(nullptr, menu_button_listener.last_source());
 }
 
 #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