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

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

 // Copyright 2015 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.
 
 #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"
 #include "chrome/browser/extensions/extension_toolbar_model.h"
 #include "chrome/browser/ui/browser_window.h"
 #import "chrome/browser/ui/cocoa/browser_window_controller.h"
 #import "chrome/browser/ui/cocoa/extensions/browser_action_button.h"
 #import "chrome/browser/ui/cocoa/extensions/browser_actions_container_view.h"
 #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"
 #import "ui/events/test/cocoa_test_event_utils.h"
 
 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];
   NSScreen* screen = [window screen];
   DCHECK(screen);
 
   // Converts the center position of the view into the coordinates accepted
   // by ui_controls methods.
   NSRect bounds = [view bounds];
   NSPoint center = NSMakePoint(NSMidX(bounds), NSMidY(bounds));
@@ skipping 50 matching lines @@
 @synthesize menuOpened = menuOpened_;
 @synthesize verify = verify_;
 
 @end
 
 class BrowserActionButtonUiTest : public ExtensionBrowserTest {
  protected:
   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;
   }
 
   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);
 };
 
 // Simulates a clicks on the action button in the overflow menu, and runs
 // |closure| upon completion.
 void ClickOnOverflowedAction(ToolbarController* toolbarController,
                              const base::Closure& closure) {
   WrenchMenuController* wrenchMenuController =
       [toolbarController wrenchMenuController];
   // The wrench menu should start as open (since that's where the overflowed
@@ skipping 34 matching lines @@
 // Test that opening a context menu works for both actions on the main bar and
 // actions in the overflow menu.
 IN_PROC_BROWSER_TEST_F(BrowserActionButtonUiTest,
                        ContextMenusOnMainAndOverflow) {
   // Add an extension with a browser action.
   scoped_refptr<const extensions::Extension> extension =
       extensions::extension_action_test_util::CreateActionExtension(
           "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);
 
   // Stub out the action button's normal context menu with a fake one so we
   // can track when it opens.
   base::scoped_nsobject<NSMenu> testContextMenu(
         [[NSMenu alloc] initWithTitle:@""]);
   base::scoped_nsobject<MenuHelper> menuHelper([[MenuHelper alloc] init]);
   [testContextMenu setDelegate:menuHelper.get()];
   [actionButton setTestContextMenu:testContextMenu.get()];
@@ skipping 16 matching lines @@
     // The menu should have opened from the click.
     EXPECT_TRUE([menuHelper menuOpened]);
   }
 
   // Reset the menu helper so we can use it again.
   [menuHelper setMenuOpened:NO];
   [menuHelper setVerify:base::Bind(
       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).
     EXPECT_FALSE([menuHelper menuOpened]);
     base::RunLoop runLoop;
     // Click on the wrench menu, and pass in a callback to continue the test
     // in ClickOnOverflowedAction (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(&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
     // action button, not the overflow's. Since Cocoa doesn't support running
     // a menu-within-a-menu, this is what has to happen.
     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);
+}