(Mac) Display the bookmark bar off-the-side menu on mouse down. Also align the menu.

(Basically, make it like Safari's button.)

We make the button into a |MenuButton|, attach a fixed menu to it in the
nib, and make the |BookmarkBarController| the delegate for this menu so
that we can update the menu just before it's displayed.

As a side effect, also fixes bug 20813.

BUG=21093,20813
TEST=Make sure the bookmark bar off-the-side button works properly under a variety of circumstances, making sure the menu is properly aligned, etc.

Review URL: http://codereview.chromium.org/199024

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@25507 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 82 insertions, 17 deletions

diff --git a/chrome/browser/cocoa/bookmark_bar_controller.h b/chrome/browser/cocoa/bookmark_bar_controller.h
index a2ae151..548210e 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.h
+++ b/chrome/browser/cocoa/bookmark_bar_controller.h
@@ -18,6 +18,7 @@ class BookmarkModel;
 class BookmarkNode;
 @class BookmarkBarView;
 class GURL;
+@class MenuButton;
 class Profile;
 class PrefService;
 @protocol ViewResizer;
@@ -68,7 +69,7 @@ class PrefService;
   id<BookmarkURLOpener> urlDelegate_;  // weak
 
   IBOutlet NSView* buttonView_;
-  IBOutlet NSButton* offTheSideButton_;
+  IBOutlet MenuButton* offTheSideButton_;
   IBOutlet NSMenu* buttonContextMenu_;
 }
 
@@ -98,7 +99,6 @@ class PrefService;
 // From a button, ...
 - (IBAction)openBookmark:(id)sender;
 - (IBAction)openFolderMenuFromButton:(id)sender;
-- (IBAction)openOffTheSideMenuFromButton:(id)sender;
 // From a context menu over the button, ...
 - (IBAction)openBookmarkInNewForegroundTab:(id)sender;
 - (IBAction)openBookmarkInNewWindow:(id)sender;
@@ -148,6 +148,8 @@ class PrefService;
 - (NSMenu *)menuForFolderNode:(const BookmarkNode*)node;
 - (int64)nodeIdFromMenuTag:(int32)tag;
 - (int32)menuTagFromNodeId:(int64)menuid;
+- (void)buildOffTheSideMenu;
+- (NSMenu*)offTheSideMenu;
 @end
 
 #endif  // CHROME_BROWSER_COCOA_BOOKMARK_BAR_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/bookmark_bar_controller.mm b/chrome/browser/cocoa/bookmark_bar_controller.mm
index e8e5daf..a8cac69 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller.mm
@@ -15,6 +15,7 @@
 #import "chrome/browser/cocoa/bookmark_name_folder_controller.h"
 #import "chrome/browser/cocoa/bookmark_menu_cocoa_controller.h"
 #import "chrome/browser/cocoa/event_utils.h"
+#import "chrome/browser/cocoa/menu_button.h"
 #import "chrome/browser/cocoa/view_resizer.h"
 #include "chrome/browser/cocoa/nsimage_cache.h"
 #include "chrome/browser/profile.h"
@@ -101,6 +102,8 @@ const CGFloat kBookmarkHorizontalPadding = 1.0;
        selector:@selector(frameDidChange)
            name:NSViewFrameDidChangeNotification
          object:[self view]];
+
+  DCHECK([offTheSideButton_ menu]);
 }
 
 - (void)showIfNeeded {
@@ -294,28 +297,41 @@ const CGFloat kBookmarkHorizontalPadding = 1.0;
   }
 }
 
-// TODO(jrg): cache the menu so we don't need to build it every time.
+// Rebuild the off-the-side menu, taking into account which buttons are
+// displayed.
+// TODO(jrg,viettrungluu): only (re)build the menu when necessary.
 // TODO(jrg): if we get smarter such that we don't even bother
 //   creating buttons which aren't visible, we'll need to be smarter
 //   here.
-- (IBAction)openOffTheSideMenuFromButton:(id)sender {
-  scoped_nsobject<NSMenu> menu([[NSMenu alloc] initWithTitle:@""]);
+- (void)buildOffTheSideMenu {
+  NSMenu* menu = [self offTheSideMenu];
+  DCHECK(menu);
+
+  // Remove old menu items (backwards order is as good as any); leave the
+  // blank one at position 0 (see menu_button.h).
+  for (NSInteger i = [menu numberOfItems] - 1; i >= 1 ; i--)
+    [menu removeItemAtIndex:i];
+
+  // Add items corresponding to buttons which aren't displayed or are only
+  // partly displayed.
   for (NSButton* each_button in buttons_.get()) {
     if (NSMaxX([each_button frame]) >
         NSMaxX([[each_button superview] frame])) {
-      [self addNode:[self nodeFromButton:each_button] toMenu:menu.get()];
+      [self addNode:[self nodeFromButton:each_button] toMenu:menu];
     }
   }
+}
 
-  // TODO(jrg): once we disable the button when the menu should be
-  // empty, remove this 'helper'.
-  if (![menu numberOfItems]) {
-    [self tagEmptyMenu:menu];
-  }
+// Get the off-the-side menu.
+- (NSMenu*)offTheSideMenu {
+  return [offTheSideButton_ menu];
+}
 
-  [NSMenu popUpContextMenu:menu
-                 withEvent:[NSApp currentEvent]
-                   forView:sender];
+// Called by any menus which have set us as their delegate (right now just the
+// off-the-side menu?).
+- (void)menuNeedsUpdate:(NSMenu*)menu {
+  if (menu == [self offTheSideMenu])
+    [self buildOffTheSideMenu];
 }
 
 // As a convention we set the menu's delegate to be the button's cell
diff --git a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
index 55c4c18..7799e5c 100644
--- a/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bar_controller_unittest.mm
@@ -524,6 +524,49 @@ TEST_F(BookmarkBarControllerTest, MiddleClick) {
   [bar_ setUrlDelegate:nil];
 }
 
+TEST_F(BookmarkBarControllerTest, TestBuildOffTheSideMenu) {
+  BookmarkModel* model = helper_.profile()->GetBookmarkModel();
+  NSMenu* menu = [bar_ offTheSideMenu];
+  ASSERT_TRUE(menu);
+
+  // The bookmark bar should start out with nothing.
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+
+  // Make sure things work when there's nothing. Note that there should always
+  // be a blank first menu item.
+  [bar_ buildOffTheSideMenu];
+  EXPECT_EQ(1, [menu numberOfItems]);
+
+  // We add lots of bookmarks. At first, we expect nothing to be added to the
+  // off-the-side menu. But once they start getting added, we expect the
+  // remaining ones to be added too. We expect a reasonably substantial number
+  // of items to be added by the end.
+  int num_off_the_side = 0;
+  for (int i = 0; i < 50; i++) {
+    const BookmarkNode* parent = model->GetBookmarkBarNode();
+    model->AddURL(parent, parent->GetChildCount(),
+                  L"very wide title",
+                  GURL("http://www.foobar.com/"));
+    [bar_ buildOffTheSideMenu];
+
+    if (num_off_the_side) {
+      num_off_the_side++;
+      EXPECT_EQ(1 + num_off_the_side, [menu numberOfItems]);
+    } else {
+      EXPECT_TRUE([menu numberOfItems] == 1 || [menu numberOfItems] == 2);
+      if ([menu numberOfItems] == 2)
+        num_off_the_side++;
+    }
+  }
+  EXPECT_GE(num_off_the_side, 20);
+
+  // Reset, and check that the built menu is "empty" again.
+  [bar_ clearBookmarkBar];
+  EXPECT_EQ(0U, [[bar_ buttons] count]);
+  [bar_ buildOffTheSideMenu];
+  EXPECT_EQ(1, [menu numberOfItems]);
+}
+
 // Cannot test these methods since they simply call a single static
 // method, BookmarkEditor::Show(), which is impossible to mock.
 // editBookmark:, addPage:
diff --git a/chrome/browser/cocoa/menu_button.h b/chrome/browser/cocoa/menu_button.h
index 92b0569..1fe6853 100644
--- a/chrome/browser/cocoa/menu_button.h
+++ b/chrome/browser/cocoa/menu_button.h
@@ -13,10 +13,14 @@
 @interface MenuButton : NSButton {
  @private
   IBOutlet NSMenu* menu_;
-  BOOL openAtRight_;
 }
 
-// The menu to display.
+// The menu to display. Note that it should have no (i.e., a blank) title and
+// that the 0-th entry should be blank (and won't be displayed). (This is
+// because we use a pulldown list, for which Cocoa uses the 0-th item as "title"
+// in the button. This might change if we ever switch to a pop-up. Our direct
+// use of the given NSMenu object means that the one can set and use NSMenu's
+// delegate as usual.)
 @property(assign, nonatomic) NSMenu* menu;
 
 @end  // @interface MenuButton
diff --git a/chrome/browser/cocoa/menu_button.mm b/chrome/browser/cocoa/menu_button.mm
index aed8034..23d5d0e 100644
--- a/chrome/browser/cocoa/menu_button.mm
+++ b/chrome/browser/cocoa/menu_button.mm
@@ -54,7 +54,7 @@
 // Reset various settings of the button and its associated |ClickHoldButtonCell|
 // to the standard state which provides reasonable defaults.
 - (void)resetToDefaults {
-  id cell = [self cell];
+  ClickHoldButtonCell* cell = [self cell];
   DCHECK([cell isKindOfClass:[ClickHoldButtonCell class]]);
   [cell setEnableClickHold:YES];
   [cell setClickHoldTimeout:0.0];       // Make menu trigger immediately.