[Extensions Mac UI] Fix more bugs in the action overflow container

chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm

Index: chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
diff --git a/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm b/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
index 55149f31f3c1cc25dd0412b32de4ee0eed1bf768..6c0b8197c528901c388b0c00b12db6ae23ecb3a3 100644
--- a/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
+++ b/chrome/browser/ui/cocoa/extensions/browser_action_button_interactive_uitest.mm
@@ -5,6 +5,8 @@
 #import <Cocoa/Cocoa.h>
 
 #include "base/memory/scoped_ptr.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_action_test_util.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -16,6 +18,9 @@
 #import "chrome/browser/ui/cocoa/extensions/browser_actions_controller.h"
 #import "chrome/browser/ui/cocoa/toolbar/toolbar_controller.h"
 #import "chrome/browser/ui/cocoa/wrench_menu/wrench_menu_controller.h"
+#include "chrome/browser/ui/global_error/global_error.h"
+#include "chrome/browser/ui/global_error/global_error_service.h"
+#include "chrome/browser/ui/global_error/global_error_service_factory.h"
 #include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "extensions/common/feature_switch.h"
@@ -23,6 +28,36 @@
 
 namespace {
 
+const int kMenuPadding = 26;
+
+// A simple error class that has a menu item.
+class MenuError : public GlobalError {
+ public:
+  MenuError() {}
+  ~MenuError() override {}
+
+  bool HasMenuItem() override { return true; }
+  int MenuItemCommandID() override {
+    // An arbitrary high id so that it's not taken.

[Avi (use Gerrit), 2015/07/30 02:50:35]

I'm sure one day you will trace down some weird bug to this function and laugh
and cry.

[Devlin, 2015/07/31 19:48:30]

This should only be used when the menu item is used, so I think we're okay.  But
there is always that possibility. :)

+    return 65536;
+  }
+  base::string16 MenuItemLabel() override {
+    const char kErrorMessage[] =
+        "This is a really long error message that will cause the wrench menu "
+        " to have increased width";

[Avi (use Gerrit), 2015/07/30 02:50:35]

Did you intend that double space? Drop the space before "to" since you already
have one at the end of the previous line.

[Devlin, 2015/07/31 19:48:30]

Done.

+    return base::ASCIIToUTF16(kErrorMessage);
+  }
+  void ExecuteMenuItem(Browser* browser) override {}
+
+  bool HasBubbleView() override { return false; }
+  bool HasShownBubbleView() override { return false; }
+  void ShowBubbleView(Browser* browser) override {}
+  GlobalErrorBubbleViewBase* GetBubbleView() override { return nullptr; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MenuError);
+};
+
 // Returns the center point for a particular |view|.
 NSPoint GetCenterPoint(NSView* view) {
   NSWindow* window = [view window];
@@ -93,7 +128,20 @@ class BrowserActionButtonUiTest : public ExtensionBrowserTest {
   BrowserActionButtonUiTest() {}
   ~BrowserActionButtonUiTest() override {}
 
+  void SetUpOnMainThread() override {
+    ExtensionBrowserTest::SetUpOnMainThread();
+    toolbarController_ =
+        [[BrowserWindowController
+            browserWindowControllerForWindow:browser()->
+                window()->GetNativeWindow()]
+            toolbarController];
+    ASSERT_TRUE(toolbarController_);
+    wrenchMenuController_ = [toolbarController_ wrenchMenuController];
+    model_ = extensions::ExtensionToolbarModel::Get(profile());
+  }
+
   void SetUpCommandLine(base::CommandLine* command_line) override {
+    ExtensionBrowserTest::SetUpCommandLine(command_line);
     enable_redesign_.reset(new extensions::FeatureSwitch::ScopedOverride(
         extensions::FeatureSwitch::extension_action_redesign(), true));
     ToolbarActionsBar::disable_animations_for_testing_ = true;
@@ -102,11 +150,21 @@ class BrowserActionButtonUiTest : public ExtensionBrowserTest {
   void TearDownOnMainThread() override {
     enable_redesign_.reset();
     ToolbarActionsBar::disable_animations_for_testing_ = false;
+    ExtensionBrowserTest::TearDownOnMainThread();
   }
 
+  ToolbarController* toolbarController() { return toolbarController_; }
+  WrenchMenuController* wrenchMenuController() { return wrenchMenuController_; }
+  extensions::ExtensionToolbarModel* model() { return model_; }
+  NSView* wrenchButton() { return [toolbarController_ wrenchButton]; }
+
  private:
   scoped_ptr<extensions::FeatureSwitch::ScopedOverride> enable_redesign_;
 
+  ToolbarController* toolbarController_ = nil;
+  WrenchMenuController* wrenchMenuController_ = nil;
+  extensions::ExtensionToolbarModel* model_ = nil;

[Avi (use Gerrit), 2015/07/30 02:50:35]

nullptr for C++ objects, nil for ObjC ones.

[Devlin, 2015/07/31 19:48:30]

Heh, did that at first, and thought it looked so weird, and wasn't sure of the
rule.  Now I know. :)  Done.

+
   DISALLOW_COPY_AND_ASSIGN(BrowserActionButtonUiTest);
 };
 
@@ -161,19 +219,10 @@ IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest,
           "browser_action",
           extensions::extension_action_test_util::BROWSER_ACTION);
   extension_service()->AddExtension(extension.get());
-  extensions::ExtensionToolbarModel* model =
-      extensions::ExtensionToolbarModel::Get(profile());
-  ASSERT_EQ(1u, model->toolbar_items().size());
-
-  ToolbarController* toolbarController =
-      [[BrowserWindowController
-          browserWindowControllerForWindow:browser()->
-              window()->GetNativeWindow()]
-          toolbarController];
-  ASSERT_TRUE(toolbarController);
+  ASSERT_EQ(1u, model()->toolbar_items().size());
 
   BrowserActionsController* actionsController =
-      [toolbarController browserActionsController];
+      [toolbarController() browserActionsController];
   BrowserActionButton* actionButton = [actionsController buttonWithIndex:0];
   ASSERT_TRUE(actionButton);
 
@@ -210,13 +259,11 @@ IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest,
       CheckActionIsPoppedOut, actionsController, actionButton)];
 
   // Shrink the visible count to be 0. This should hide the action button.
-  model->SetVisibleIconCount(0);
+  model()->SetVisibleIconCount(0);
   EXPECT_EQ(nil, [actionButton superview]);
 
   // Move the mouse over the wrench button.
-  NSView* wrenchButton = [toolbarController wrenchButton];
-  ASSERT_TRUE(wrenchButton);
-  MoveMouseToCenter(wrenchButton);
+  MoveMouseToCenter(wrenchButton());
 
   {
     // No menu yet (on the browser action).
@@ -228,7 +275,7 @@ IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest,
     ui_controls::SendMouseEventsNotifyWhenDone(
         ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP,
         base::Bind(&ClickOnOverflowedAction,
-                   base::Unretained(toolbarController),
+                   base::Unretained(toolbarController()),
                    runLoop.QuitClosure()));
     runLoop.Run();
     // The menu should have opened. Note that the menu opened on the main bar's
@@ -237,3 +284,119 @@ IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest,
     EXPECT_TRUE([menuHelper menuOpened]);
   }
 }
+
+// Checks the layout of the overflow bar in the wrench menu.
+void CheckWrenchMenuLayout(ToolbarController* toolbarController,
+                           int overflowStartIndex,
+                           const base::Closure& closure) {
+  WrenchMenuController* wrenchMenuController =
+      [toolbarController wrenchMenuController];
+  // The wrench menu should start as open (since that's where the overflowed
+  // actions are).
+  EXPECT_TRUE([wrenchMenuController isMenuOpen]);
+  BrowserActionsController* overflowController =
+      [wrenchMenuController browserActionsController];
+  ASSERT_TRUE(overflowController);
+
+  ToolbarActionsBar* overflowBar = [overflowController toolbarActionsBar];
+  BrowserActionsContainerView* overflowContainer =
+      [overflowController containerView];
+  NSMenu* menu = [wrenchMenuController menu];
+
+  // The overflow container should be within the bounds of the wrench menu, as
+  // should its parents.
+  int menu_width = [menu size].width;
+  NSRect frame = [overflowContainer frame];
+  EXPECT_GE(NSMinX(frame), 0);
+  // Hierarchy: The overflow container is owned by two different views in the
+  // wrench menu. Each should have a min x of >= 0.
+  EXPECT_GE(NSMinX([[overflowContainer superview] frame]), 0);
+  EXPECT_GE(NSMinX([[[overflowContainer superview] superview] frame]), 0);
+  // The overflow container should fully fit in the wrench menu, including the
+  // space taken away for padding, and should have its desired size.
+  EXPECT_LE(NSWidth(frame), menu_width - kMenuPadding);
+  EXPECT_EQ(NSWidth(frame), overflowBar->GetPreferredSize().width());
+
+  // Every button that has an index lower than the overflow start index (i.e.,
+  // every button on the main toolbar) should not be attached to a view.
+  for (int i = 0; i < overflowStartIndex; ++i) {
+    BrowserActionButton* button = [overflowController buttonWithIndex:i];
+    EXPECT_FALSE([button superview]);
+    EXPECT_GE(0, NSMaxX([button frame]));
+  }
+
+  // Every other button should be attached to a view, and should be at the
+  // proper bounds. Calculating each button's proper bounds here would just be
+  // a duplication of the logic in the method, but we can test that each button
+  // a) is within the overflow container's bounds, and
+  // b) doesn't intersect with another button.
+  // If both of those are true, then we're probably good.
+  for (NSUInteger i = overflowStartIndex;
+       i < [overflowController buttonCount]; ++i) {
+    BrowserActionButton* button = [overflowController buttonWithIndex:i];
+    EXPECT_TRUE([button superview]);
+    EXPECT_TRUE(NSContainsRect([overflowContainer bounds], [button frame]));
+    for (NSUInteger j = 0; j < i; ++j) {
+      EXPECT_FALSE(
+          NSContainsRect([[overflowController buttonWithIndex:j] frame],
+                         [button frame]));
+    }
+  }
+
+  // Close the wrench menu.
+  [wrenchMenuController cancel];
+  EXPECT_FALSE([wrenchMenuController isMenuOpen]);
+  base::MessageLoop::current()->PostTask(FROM_HERE, closure);
+}
+
+// Tests the layout of the overflow container in the wrench menu.
+IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest, TestOverflowContainerLayout) {
+  // Add a bunch of extensions - enough to trigger multiple rows in the overflow
+  // menu.
+  const int kNumExtensions = 12;
+  for (int i = 0; i < kNumExtensions; ++i) {
+    scoped_refptr<const extensions::Extension> extension =
+        extensions::extension_action_test_util::CreateActionExtension(
+            base::StringPrintf("extension%d", i),
+            extensions::extension_action_test_util::BROWSER_ACTION);
+    extension_service()->AddExtension(extension.get());
+  }
+  ASSERT_EQ(kNumExtensions, static_cast<int>(model()->toolbar_items().size()));
+
+  // A helper function to open the wrench menu and call the check function.
+  auto resizeAndActivateWrench = [this](int visible_count) {
+    model()->SetVisibleIconCount(kNumExtensions - visible_count);
+    MoveMouseToCenter(wrenchButton());
+
+    {
+      base::RunLoop runLoop;
+      // Click on the wrench menu, and pass in a callback to continue the test
+      // in CheckWrenchMenuLayout (due to the blocking nature of Cocoa menus,
+      // passing in runLoop.QuitClosure() is not sufficient here.)
+      ui_controls::SendMouseEventsNotifyWhenDone(
+          ui_controls::LEFT, ui_controls::DOWN | ui_controls::UP,
+          base::Bind(&CheckWrenchMenuLayout,
+                     base::Unretained(toolbarController()),
+                     kNumExtensions - visible_count,
+                     runLoop.QuitClosure()));
+      runLoop.Run();
+    }
+  };
+
+  // Test the layout with gradually more extensions hidden.
+  for (int i = 1; i <= kNumExtensions; ++i)
+    resizeAndActivateWrench(i);
+
+  // Adding a global error adjusts the wrench menu size, and has been known
+  // to mess up the overflow container's bounds (crbug.com/511326).
+  GlobalErrorService* error_service =
+      GlobalErrorServiceFactory::GetForProfile(profile());
+  error_service->AddGlobalError(new MenuError());
+
+  // It's probably excessive to test every level of the overflow here. Test
+  // having all actions overflowed, some actions overflowed, and one action
+  // overflowed.
+  resizeAndActivateWrench(kNumExtensions);
+  resizeAndActivateWrench(kNumExtensions / 2);
+  resizeAndActivateWrench(1);
+}