Bookmark menu work. Notes:

- "add bookmark" menu item enable state (e.g. disabled if no windows)
- bookmark menus built dynamically (like before)
- bookmark menus rebuild when a bookmark is added/removed
- bookmark menus take the current browser to where you want to go
- works with multiple windows (main window goes to bookmark location)
- works with no windows (bookmarks open a new window)

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

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

17 files changed, 333 insertions, 65 deletions

diff --git a/chrome/app/chrome_dll_resource.h b/chrome/app/chrome_dll_resource.h
index b981be7..d89c825 100644
--- a/chrome/app/chrome_dll_resource.h
+++ b/chrome/app/chrome_dll_resource.h
@@ -200,7 +200,5 @@
 
 // Identifiers for platform-specific items.
 // Placed in a common file to help insure they never collide.
-#define IDC_BOOKMARK_MENU               43000  // OSX only
-#define IDC_BOOKMARK_MENUITEM_BASE      43001  // OSX only
-// Numbers 43002-43998 reserved for menu items
-#define IDC_BOOKMARK_MENUITEM_MAX       43999  // OSX only
+#define IDC_BOOKMARK_MENU               43000     // OSX only
+
diff --git a/chrome/app/nibs/en.lproj/MainMenu.xib b/chrome/app/nibs/en.lproj/MainMenu.xib
index dd592bf..1cb6c61 100644
--- a/chrome/app/nibs/en.lproj/MainMenu.xib
+++ b/chrome/app/nibs/en.lproj/MainMenu.xib
@@ -882,6 +882,7 @@
 									<int key="NSMnemonicLoc">2147483647</int>
 									<reference key="NSOnImage" ref="353210768"/>
 									<reference key="NSMixedImage" ref="549394948"/>
+									<int key="NSTag">35000</int>
 								</object>
 							</object>
 						</object>
@@ -2457,7 +2458,7 @@
 					<reference ref="9"/>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<reference ref="9"/>
-					<string>{{262, 631}, {243, 263}}</string>
+					<string>{{264, 632}, {243, 263}}</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<reference ref="9"/>
 					<string>{{197, 734}, {243, 243}}</string>
@@ -2557,7 +2558,7 @@
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
-					<string>{{425, 862}, {230, 33}}</string>
+					<string>{{425, 861}, {230, 33}}</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<reference ref="9"/>
@@ -2568,7 +2569,7 @@
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
-					<string>{{356, 801}, {188, 93}}</string>
+					<string>{{358, 801}, {188, 93}}</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
 					<string>com.apple.InterfaceBuilder.CocoaPlugin</string>
diff --git a/chrome/browser/app_controller_mac.h b/chrome/browser/app_controller_mac.h
index db562a3..b64fe7b 100644
--- a/chrome/browser/app_controller_mac.h
+++ b/chrome/browser/app_controller_mac.h
@@ -7,7 +7,9 @@
 
 #import <Cocoa/Cocoa.h>
 
+class BookmarkMenuBridge;
 class CommandUpdater;
+class Profile;
 
 // The application controller object, created by loading the MainMenu nib.
 // This handles things like responding to menus when there are no windows
@@ -15,9 +17,15 @@ class CommandUpdater;
 @interface AppController : NSObject<NSUserInterfaceValidations> {
  @public
   CommandUpdater* menuState_;  // strong ref
+ @private
+  // Management of the bookmark menu which spans across all windows
+  // (and Browser*s).  This is dynamically allocated to keep objc
+  // happy.
+  BookmarkMenuBridge* bookmarkMenuBridge_;
 }
 
 - (IBAction)quit:(id)sender;
+- (Profile*)defaultProfile;
 
 @end
 
diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index 9bd9de2..98ed64b 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -9,6 +9,7 @@
 #import "chrome/browser/browser.h"
 #import "chrome/browser/browser_list.h"
 #include "chrome/browser/browser_shutdown.h"
+#import "chrome/browser/cocoa/bookmark_menu_bridge.h"
 #import "chrome/browser/command_updater.h"
 #import "chrome/browser/profile_manager.h"
 #import "chrome/common/temp_scaffolding_stubs.h"
@@ -22,6 +23,7 @@
 - (void)awakeFromNib {
   // Set up the command updater for when there are no windows open
   [self initMenuState];
+  bookmarkMenuBridge_ = new BookmarkMenuBridge();
 }
 
 - (void)applicationDidFinishLaunching:(NSNotification*)notify {
@@ -33,6 +35,7 @@
 }
 
 - (void)dealloc {
+  delete bookmarkMenuBridge_;
   delete menuState_;
   [super dealloc];
 }
@@ -87,8 +90,7 @@
 // command is supported and doesn't check, otherwise it would have been disabled
 // in the UI in validateUserInterfaceItem:.
 - (void)commandDispatch:(id)sender {
-  // How to get the profile created on line 314 of browser_main? Ugh. TODO:FIXME
-  Profile* default_profile = *g_browser_process->profile_manager()->begin();
+  Profile* default_profile = [self defaultProfile];
 
   NSInteger tag = [sender tag];
   switch (tag) {
@@ -108,4 +110,16 @@
   // TODO(pinkerton): ...more to come...
 }
 
+- (Profile*)defaultProfile {
+  // Only expected to return NULL in a unit test (if g_browser_process
+  // is a TestingBrowserProcess).
+  // TODO(jrg): DCHECK() to confirm that.
+  // TODO(jrg): Find a better way to get the "default" profile.
+  if (g_browser_process->profile_manager())
+    return *g_browser_process->profile_manager()->begin();
+  return NULL;
+
+}
+
+
 @end
diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc
index f583737..6665d15 100644
--- a/chrome/browser/browser_list.cc
+++ b/chrome/browser/browser_list.cc
@@ -180,6 +180,9 @@ BrowserList::list_type BrowserList::last_active_browsers_;
 void BrowserList::SetLastActive(Browser* browser) {
   RemoveBrowserFrom(browser, &last_active_browsers_);
   last_active_browsers_.push_back(browser);
+
+  for (int i = 0; i < static_cast<int>(observers_.size()); i++)
+    observers_[i]->OnBrowserSetLastActive(browser);
 }
 
 // static
diff --git a/chrome/browser/browser_list.h b/chrome/browser/browser_list.h
index 44246ac..63ea28d 100644
--- a/chrome/browser/browser_list.h
+++ b/chrome/browser/browser_list.h
@@ -27,6 +27,9 @@ class BrowserList {
 
     // Called immediately before a browser is removed from the list
     virtual void OnBrowserRemoving(const Browser* browser) = 0;
+
+    // Called immediately after a browser is set active (SetLastActive)
+    virtual void OnBrowserSetLastActive(const Browser* browser) { };
   };
 
   // Adds and removes browsers from the global list. The browser object should
diff --git a/chrome/browser/cocoa/bookmark_menu_bridge.h b/chrome/browser/cocoa/bookmark_menu_bridge.h
index 81ff98fc..23c1d47 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge.h
+++ b/chrome/browser/cocoa/bookmark_menu_bridge.h
@@ -2,28 +2,40 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// C++ controller for the bookmark menu.  When bookmarks are changed,
-// this class takes care of updating Cocoa bookmark menus.  This is
-// not named BookmarkMenuController to help avoid confusion between
-// languages.  This class needs to be C++, not ObjC, since it derives
-// from BookmarkModelObserver.
+// C++ controller for the bookmark menu; one per AppController (which
+// means there is only one).  When bookmarks are changed, this class
+// takes care of updating Cocoa bookmark menus.  This is not named
+// BookmarkMenuController to help avoid confusion between languages.
+// This class needs to be C++, not ObjC, since it derives from
+// BookmarkModelObserver.
+//
+// Most Chromium Cocoa menu items are static from a nib (e.g. New
+// Tab), but may be enabled/disabled under certain circumstances
+// (e.g. Cut and Paste).  In addition, most Cocoa menu items have
+// firstResponder: as a target.  Unusually, bookmark menu items are
+// created dynamically.  They also have a target of
+// BookmarkMenuCocoaController instead of firstResponder.
+// See BookmarkMenuBridge::AddNodeToMenu()).
 
 #ifndef CHROME_BROWSER_COCOA_BOOKMARK_MENU_BRIDGE_H_
 #define CHROME_BROWSER_COCOA_BOOKMARK_MENU_BRIDGE_H_
 
 #include "chrome/browser/bookmarks/bookmark_model.h"
-
+#include "chrome/browser/browser_list.h"
 
 class Browser;
 @class NSMenu;
+@class BookmarkMenuCocoaController;
 
-class BookmarkMenuBridge : public BookmarkModelObserver {
+class BookmarkMenuBridge : public BookmarkModelObserver,
+                           public BrowserList::Observer {
  public:
-  BookmarkMenuBridge(Browser* browser);
+  BookmarkMenuBridge();
   ~BookmarkMenuBridge();
 
   // Overridden from BookmarkModelObserver
   virtual void Loaded(BookmarkModel* model);
+  virtual void BookmarkModelBeingDeleted(BookmarkModel* model);
   virtual void BookmarkNodeMoved(BookmarkModel* model,
                                  BookmarkNode* old_parent,
                                  int old_index,
@@ -39,6 +51,15 @@ class BookmarkMenuBridge : public BookmarkModelObserver {
   virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
                                              BookmarkNode* node);
 
+  // Overridden from BrowserList::Observer
+  virtual void OnBrowserAdded(const Browser* browser);
+  virtual void OnBrowserRemoving(const Browser* browser);
+  virtual void OnBrowserSetLastActive(const Browser* browser);
+
+  // I wish I has a "friend @class" construct.
+  BookmarkModel* GetBookmarkModel();
+  Profile* GetDefaultProfile();
+
  protected:
   // Clear all bookmarks from the given bookmark menu.
   void ClearBookmarkMenu(NSMenu* menu);
@@ -48,9 +69,13 @@ class BookmarkMenuBridge : public BookmarkModelObserver {
   // TODO(jrg): add a counter to enforce maximum nodes added
   void AddNodeToMenu(BookmarkNode* node, NSMenu* menu);
 
+  // Return the Bookmark menu.
+  NSMenu* BookmarkMenu();
+
  private:
   friend class BookmarkMenuBridgeTest;
-  Browser* browser_;
+
+  BookmarkMenuCocoaController* controller_;  // strong
 };
 
 #endif  // CHROME_BROWSER_COCOA_BOOKMARK_MENU_BRIDGE_H_
diff --git a/chrome/browser/cocoa/bookmark_menu_bridge.mm b/chrome/browser/cocoa/bookmark_menu_bridge.mm
index ca78da3..8d32150 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge.mm
+++ b/chrome/browser/cocoa/bookmark_menu_bridge.mm
@@ -6,29 +6,46 @@
 #import <AppKit/AppKit.h>
 #include "base/sys_string_conversions.h"
 #include "chrome/app/chrome_dll_resource.h"  // IDC_BOOKMARK_MENU
+#import "chrome/browser/app_controller_mac.h"
 #include "chrome/browser/browser.h"
+#include "chrome/browser/browser_list.h"
+#import "chrome/browser/cocoa/bookmark_menu_cocoa_controller.h"
 #include "chrome/browser/profile.h"
+#include "chrome/browser/profile_manager.h"
 
-
-BookmarkMenuBridge::BookmarkMenuBridge(Browser* browser) : browser_(browser) {
-  browser_->profile()->GetBookmarkModel()->AddObserver(this);
+BookmarkMenuBridge::BookmarkMenuBridge()
+    : controller_([[BookmarkMenuCocoaController alloc] initWithBridge:this]) {
+  BrowserList::AddObserver(this);
 }
 
 BookmarkMenuBridge::~BookmarkMenuBridge() {
-  browser_->profile()->GetBookmarkModel()->RemoveObserver(this);
+  GetBookmarkModel()->RemoveObserver(this);
+  [controller_ release];
 }
 
-void BookmarkMenuBridge::Loaded(BookmarkModel* model) {
-  NSMenu *bookmark_menu = [[[NSApp mainMenu] itemWithTag:IDC_BOOKMARK_MENU]
+NSMenu* BookmarkMenuBridge::BookmarkMenu() {
+  NSMenu* bookmark_menu = [[[NSApp mainMenu] itemWithTag:IDC_BOOKMARK_MENU]
                             submenu];
+  return bookmark_menu;
+}
+
+void BookmarkMenuBridge::Loaded(BookmarkModel* model) {
+  NSMenu* bookmark_menu = BookmarkMenu();
   if (bookmark_menu == nil)
     return;
 
-  this->ClearBookmarkMenu(bookmark_menu);
+  ClearBookmarkMenu(bookmark_menu);
 
-  // TODO(jrg): limit the number of bookmarks in the menubar to max_nodes
-  // int max_nodes = IDC_BOOKMARK_MENUITEM_MAX - IDC_BOOKMARK_MENUITEM_BASE;
-  this->AddNodeToMenu(model->GetBookmarkBarNode(), bookmark_menu);
+  // TODO(jrg): limit the number of bookmarks in the menubar?
+  AddNodeToMenu(model->GetBookmarkBarNode(), bookmark_menu);
+}
+
+void BookmarkMenuBridge::BookmarkModelBeingDeleted(BookmarkModel* model) {
+  NSMenu* bookmark_menu = BookmarkMenu();
+  if (bookmark_menu == nil)
+    return;
+
+  ClearBookmarkMenu(bookmark_menu);
 }
 
 void BookmarkMenuBridge::BookmarkNodeMoved(BookmarkModel* model,
@@ -37,22 +54,20 @@ void BookmarkMenuBridge::BookmarkNodeMoved(BookmarkModel* model,
                                            BookmarkNode* new_parent,
                                            int new_index) {
   // TODO(jrg): this is brute force; perhaps we should be nicer.
-  this->Loaded(model);
+  Loaded(model);
 }
 
 void BookmarkMenuBridge::BookmarkNodeAdded(BookmarkModel* model,
                                            BookmarkNode* parent,
                                            int index) {
-
   // TODO(jrg): this is brute force; perhaps we should be nicer.
-  this->Loaded(model);
+  Loaded(model);
 }
 
 void BookmarkMenuBridge::BookmarkNodeChanged(BookmarkModel* model,
                                              BookmarkNode* node) {
-
   // TODO(jrg): this is brute force; perhaps we should be nicer.
-  this->Loaded(model);
+  Loaded(model);
 }
 
 void BookmarkMenuBridge::BookmarkNodeFavIconLoaded(BookmarkModel* model,
@@ -65,7 +80,42 @@ void BookmarkMenuBridge::BookmarkNodeFavIconLoaded(BookmarkModel* model,
 void BookmarkMenuBridge::BookmarkNodeChildrenReordered(BookmarkModel* model,
                                                        BookmarkNode* node) {
   // TODO(jrg): this is brute force; perhaps we should be nicer.
-  this->Loaded(model);
+  Loaded(model);
+}
+
+void BookmarkMenuBridge::OnBrowserAdded(const Browser* browser) {
+  // Intentionally empty -- we don't care, but pure virtual so we must
+  // implement.
+}
+
+void BookmarkMenuBridge::OnBrowserRemoving(const Browser* browser) {
+  // Intentionally empty -- we don't care, but pure virtual so we must
+  // implement.
+}
+
+// The current browser has changed; update the bookmark menus.  For
+// our use, we only care about the first one to know when a profile is
+// complete.
+void BookmarkMenuBridge::OnBrowserSetLastActive(const Browser* browser) {
+  BrowserList::RemoveObserver(this);
+  BookmarkModel* model = GetBookmarkModel();
+  model->AddObserver(this);
+  if (model->IsLoaded())
+    Loaded(model);
+}
+
+BookmarkModel* BookmarkMenuBridge::GetBookmarkModel() {
+  // In incognito mode we use the main profile's bookmarks.
+  // Thus, we don't return browser_->profile()->GetBookmarkModel().
+  Profile* profile = GetDefaultProfile();
+  // In unit tests, there is no default profile.
+  // TODO(jrg): refactor so we don't "allow" NULLs in non-test environments.
+  return profile ? profile->GetBookmarkModel() : NULL;
+}
+
+Profile* BookmarkMenuBridge::GetDefaultProfile() {
+  // The delegate of the main application is an AppController
+  return [[NSApp delegate] defaultProfile];
 }
 
 void BookmarkMenuBridge::ClearBookmarkMenu(NSMenu* menu) {
@@ -75,21 +125,21 @@ void BookmarkMenuBridge::ClearBookmarkMenu(NSMenu* menu) {
   // and separator)
   NSArray* items = [menu itemArray];
   for (NSMenuItem* item in items) {
-    NSInteger tag = [item tag];
-    if ((tag >= IDC_BOOKMARK_MENUITEM_BASE) &&
-        (tag < IDC_BOOKMARK_MENUITEM_MAX)) {
+    // Convention: items in the bookmark list which are bookmarks have
+    // an action of openBookmarkMenuItem:.  Also, assume all items
+    // with submenus are submenus of bookmarks.
+    if (([item action] == @selector(openBookmarkMenuItem:)) ||
+        ([item hasSubmenu])) {
+      // This will eventually [obj release] all its kids, if it has
+      // any.
       [menu removeItem:item];
-    } else if ([item hasSubmenu]) {
-      [menu removeItem:item];  // Will eventually [obj release] all its kids
     } else {
       // Not a bookmark or item with submenu, so leave it alone.
     }
   }
 }
 
-// TODO(jrg): add actions for these menu items
-void BookmarkMenuBridge::AddNodeToMenu(BookmarkNode* node,
-                                       NSMenu* menu) {
+void BookmarkMenuBridge::AddNodeToMenu(BookmarkNode* node, NSMenu* menu) {
   for (int i = 0; i < node->GetChildCount(); i++) {
     BookmarkNode* child = node->GetChild(i);
     // TODO(jrg): Should we limit the title length?
@@ -99,12 +149,14 @@ void BookmarkMenuBridge::AddNodeToMenu(BookmarkNode* node,
     NSMenuItem* item = [[[NSMenuItem alloc] initWithTitle:title
                                                    action:nil
                                             keyEquivalent:@""] autorelease];
-    [item setTag:IDC_BOOKMARK_MENUITEM_BASE];
+    [item setTarget:controller_];
+    [item setAction:@selector(openBookmarkMenuItem:)];
+    [item setTag:child->id()];
     [menu addItem:item];
     if (child->is_folder()) {
       NSMenu* submenu = [[[NSMenu alloc] initWithTitle:title] autorelease];
       [menu setSubmenu:submenu forItem:item];
-      this->AddNodeToMenu(child, submenu);  // recursive call
+      AddNodeToMenu(child, submenu);  // recursive call
     }
   }
 }
diff --git a/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm b/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
index d00f75f..48a6dc4 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
@@ -26,10 +26,11 @@ class BookmarkMenuBridgeTest : public testing::Test {
     bridge->AddNodeToMenu(root, menu);
   }
 
-  NSMenuItem* AddItemToMenu(NSMenu *menu, NSString *title, NSInteger tag) {
+  NSMenuItem* AddItemToMenu(NSMenu *menu, NSString *title, SEL selector) {
     NSMenuItem *item = [[[NSMenuItem alloc] initWithTitle:title action:NULL
                                             keyEquivalent:@""] autorelease];
-    [item setTag:tag];
+    if (selector)
+      [item setAction:selector];
     [menu addItem:item];
     return item;
   }
@@ -40,35 +41,34 @@ class BookmarkMenuBridgeTest : public testing::Test {
 
 // Test that ClearBookmarkMenu() removes all bookmark menus.
 TEST_F(BookmarkMenuBridgeTest, TestClearBookmarkMenu) {
-  Browser* browser = browser_test_helper_.GetBrowser();
-  BookmarkMenuBridge* bridge = new BookmarkMenuBridge(browser);
+  BookmarkMenuBridge* bridge = new BookmarkMenuBridge();
   EXPECT_TRUE(bridge);
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"foo"] autorelease];
 
-  AddItemToMenu(menu, @"hi mom", IDC_BOOKMARK_MENUITEM_BASE);
-  AddItemToMenu(menu, @"not", 0);
-  NSMenuItem* item = AddItemToMenu(menu, @"hi mom", 0);
+  AddItemToMenu(menu, @"hi mom", nil);
+  AddItemToMenu(menu, @"not", @selector(openBookmarkMenuItem:));
+  NSMenuItem* item = AddItemToMenu(menu, @"hi mom", nil);
   [item setSubmenu:[[[NSMenu alloc] initWithTitle:@"bar"] autorelease]];
-  AddItemToMenu(menu, @"not", 0);
+  AddItemToMenu(menu, @"not", @selector(openBookmarkMenuItem:));
+  AddItemToMenu(menu, @"zippy", @selector(length));
 
   ClearBookmarkMenu(bridge, menu);
 
-  // Make sure all IDC_BOOKMARK items are removed, and all items with
+  // Make sure all bookmark items are removed, and all items with
   // submenus removed.
   EXPECT_EQ(2, [menu numberOfItems]);
   for (NSMenuItem *item in [menu itemArray]) {
-    EXPECT_TRUE([[item title] isEqual:@"not"]);
+    EXPECT_FALSE([[item title] isEqual:@"not"]);
   }
 }
 
 // Test that AddNodeToMenu() properly adds bookmark nodes as menus,
 // including the recursive case.
 TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
-  Browser* browser = browser_test_helper_.GetBrowser();
   Profile* profile = browser_test_helper_.GetProfile();
 
-  BookmarkMenuBridge *bridge = new BookmarkMenuBridge(browser);
+  BookmarkMenuBridge *bridge = new BookmarkMenuBridge();
   EXPECT_TRUE(bridge);
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"foo"] autorelease];
@@ -92,9 +92,7 @@ TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
   EXPECT_EQ(3, [menu numberOfItems]);
   for (int x=0; x < 3; x++) {
     NSMenuItem* item = [menu itemAtIndex:x];
-    NSInteger tag = [item tag];
-    EXPECT_TRUE((tag >= IDC_BOOKMARK_MENUITEM_BASE) &&
-                (tag < IDC_BOOKMARK_MENUITEM_MAX));
+    EXPECT_EQ(@selector(openBookmarkMenuItem:), [item action]);
   }
   EXPECT_EQ(NO, [[menu itemAtIndex:0] hasSubmenu]);
   EXPECT_EQ(NO, [[menu itemAtIndex:2] hasSubmenu]);
diff --git a/chrome/browser/cocoa/bookmark_menu_cocoa_controller.h b/chrome/browser/cocoa/bookmark_menu_cocoa_controller.h
new file mode 100644
index 0000000..83e6098
--- /dev/null
+++ b/chrome/browser/cocoa/bookmark_menu_cocoa_controller.h
@@ -0,0 +1,36 @@
+// Copyright (c) 2009 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.
+
+// Controller (MVC) for the bookmark menu.
+// All bookmark menu item commands get directed here.
+// Unfortunately there is already a C++ class named BookmarkMenuController.
+
+#ifndef CHROME_BROWSER_COCOA_BOOKMARK_MENU_COCOA_CONTROLLER_H_
+#define CHROME_BROWSER_COCOA_BOOKMARK_MENU_COCOA_CONTROLLER_H_
+
+#import <Cocoa/Cocoa.h>
+
+class BookmarkNode;
+class BookmarkMenuBridge;
+
+@interface BookmarkMenuCocoaController : NSObject {
+ @private
+  BookmarkMenuBridge* bridge_;  // weak; owns me
+}
+
+- (id)initWithBridge:(BookmarkMenuBridge *)bridge;
+
+// Called by any Bookmark menu item.
+// The menu item's tag is the bookmark ID.
+- (IBAction)openBookmarkMenuItem:(id)sender;
+
+@end  // BookmarkMenuCocoaController
+
+
+@interface BookmarkMenuCocoaController (ExposedForUnitTests)
+- (BookmarkNode*)nodeForIdentifier:(int)identifier;
+- (void)openURLForNode:(BookmarkNode*)node;
+@end  // BookmarkMenuCocoaController (ExposedForUnitTests)
+
+#endif  // CHROME_BROWSER_COCOA_BOOKMARK_MENU_COCOA_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm b/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm
new file mode 100644
index 0000000..c8dd3ec
--- /dev/null
+++ b/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm
@@ -0,0 +1,56 @@
+// Copyright (c) 2009 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 "chrome/browser/browser.h"
+#import "chrome/browser/cocoa/bookmark_menu_bridge.h"
+#import "chrome/browser/cocoa/bookmark_menu_cocoa_controller.h"
+#include "chrome/browser/browser_process.h"
+#include "webkit/glue/window_open_disposition.h"  // CURRENT_TAB
+
+@implementation BookmarkMenuCocoaController
+
+- (id)initWithBridge:(BookmarkMenuBridge *)bridge {
+  if ((self = [super init])) {
+    bridge_ = bridge;
+    DCHECK(bridge_);
+  }
+  return self;
+}
+
+// Return the a BookmarkNode that has the given id (called
+// "identifier" here to avoid conflict with objc's concept of "id").
+- (BookmarkNode*)nodeForIdentifier:(int)identifier {
+  return bridge_->GetBookmarkModel()->GetNodeByID(identifier);
+}
+
+// Open the URL of the given BookmarkNode in the current tab.
+- (void)openURLForNode:(BookmarkNode*)node {
+  Browser* browser = BrowserList::GetLastActive();
+
+  if (!browser) {  // No windows open?
+    Browser::OpenEmptyWindow(bridge_->GetDefaultProfile());
+    browser = BrowserList::GetLastActive();
+  }
+  DCHECK(browser);
+  TabContents* tab_contents = browser->GetSelectedTabContents();
+  DCHECK(tab_contents);
+
+  // A TabContents is a PageNavigator, so we can OpenURL() on it.
+  tab_contents->OpenURL(node->GetURL(), GURL(), CURRENT_TAB,
+                        PageTransition::AUTO_BOOKMARK);
+}
+
+- (IBAction)openBookmarkMenuItem:(id)sender {
+  NSInteger tag = [sender tag];
+  int identifier = tag;
+  BookmarkNode* node = [self nodeForIdentifier:identifier];
+  DCHECK(node);
+  if (!node)
+    return;  // shouldn't be reached
+
+  [self openURLForNode:node];
+}
+
+@end  // BookmarkMenuCocoaController
+
diff --git a/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm b/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm
new file mode 100644
index 0000000..f8f5f53
--- /dev/null
+++ b/chrome/browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm
@@ -0,0 +1,67 @@
+// Copyright (c) 2009 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 "chrome/browser/browser.h"
+#include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/cocoa/browser_test_helper.h"
+#include "chrome/browser/cocoa/bookmark_menu_cocoa_controller.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+@interface FakeBookmarkMenuController : BookmarkMenuCocoaController {
+ @public
+  BrowserTestHelper* helper_;
+  BookmarkNode* nodes_[2];
+  BOOL opened_[2];
+}
+@end
+
+@implementation FakeBookmarkMenuController
+
+- (id)init {
+  if ((self = [super init])) {
+    helper_ = new BrowserTestHelper();
+    BookmarkModel* model = helper_->GetBrowser()->profile()->GetBookmarkModel();
+    nodes_[0] = new BookmarkNode(model, GURL("http://0.com"));
+    nodes_[1] = new BookmarkNode(model, GURL("http://1.com"));
+  }
+  return self;
+}
+
+- (void)dealloc {
+  delete nodes_[0];
+  delete nodes_[1];
+  delete helper_;
+  [super dealloc];
+}
+
+- (BookmarkNode*)nodeForIdentifier:(int)identifier {
+  if ((identifier < 0) || (identifier >= 2))
+    return NULL;
+  return nodes_[identifier];
+}
+
+- (void)openURLForNode:(BookmarkNode*)node {
+  std::string url = node->GetURL().possibly_invalid_spec();
+  if (url.find("http://0.com") != std::string::npos)
+    opened_[0] = YES;
+  if (url.find("http://1.com") != std::string::npos)
+    opened_[1] = YES;
+}
+
+@end  // FakeBookmarkMenuController
+
+
+TEST(BookmarkMenuCocoaControllerTest, TestOpenItem) {
+  FakeBookmarkMenuController *c = [[FakeBookmarkMenuController alloc] init];
+  NSMenuItem *item = [[[NSMenuItem alloc] init] autorelease];
+  for (int i = 0; i < 2; i++) {
+    [item setTag:i];
+    ASSERT_EQ(c->opened_[i], NO);
+    [c openBookmarkMenuItem:item];
+    ASSERT_NE(c->opened_[i], NO);
+  }
+  [c release];
+}
+
+
diff --git a/chrome/browser/cocoa/browser_window_cocoa.h b/chrome/browser/cocoa/browser_window_cocoa.h
index fa4661c..9e73c18 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.h
+++ b/chrome/browser/cocoa/browser_window_cocoa.h
@@ -8,7 +8,6 @@
 #include "base/scoped_ptr.h"
 #include "chrome/browser/browser_window.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
-#include "chrome/browser/cocoa/bookmark_menu_bridge.h"
 
 class Browser;
 @class BrowserWindowController;
@@ -78,7 +77,6 @@ class BrowserWindowCocoa : public BrowserWindow {
   NSWindow* window_;  // weak, owned by |controller_|
   // The status bubble manager.  Always non-NULL.
   scoped_ptr<StatusBubbleMac> status_bubble_;
-  BookmarkMenuBridge bookmark_menu_bridge_;
 };
 
 #endif  // CHROME_BROWSER_COCOA_BROWSER_WINDOW_COCOA_H_
diff --git a/chrome/browser/cocoa/browser_window_cocoa.mm b/chrome/browser/cocoa/browser_window_cocoa.mm
index 84729fb..420071a 100644
--- a/chrome/browser/cocoa/browser_window_cocoa.mm
+++ b/chrome/browser/cocoa/browser_window_cocoa.mm
@@ -12,8 +12,7 @@
 BrowserWindowCocoa::BrowserWindowCocoa(Browser* browser,
                                        BrowserWindowController* controller,
                                        NSWindow* window)
-  : browser_(browser), controller_(controller), window_(window),
-    bookmark_menu_bridge_(browser) {
+  : browser_(browser), controller_(controller), window_(window) {
   status_bubble_.reset(new StatusBubbleMac(window_));
 }
 
@@ -204,3 +203,4 @@ void BrowserWindowCocoa::DestroyBrowser() {
   // at this point the controller is dead (autoreleased), so
   // make sure we don't try to reference it any more.
 }
+
diff --git a/chrome/browser/cocoa/browser_window_controller.h b/chrome/browser/cocoa/browser_window_controller.h
index 1e1e798..9e552e6 100644
--- a/chrome/browser/cocoa/browser_window_controller.h
+++ b/chrome/browser/cocoa/browser_window_controller.h
@@ -17,9 +17,9 @@ class BrowserWindow;
 class BrowserWindowCocoa;
 class LocationBar;
 class TabContents;
-@class TabStripView;
 @class TabContentsController;
 @class TabStripController;
+@class TabStripView;
 
 @interface BrowserWindowController :
     TabWindowController<NSUserInterfaceValidations> {
diff --git a/chrome/browser/cocoa/browser_window_controller.mm b/chrome/browser/cocoa/browser_window_controller.mm
index b20f30c..95452b2 100644
--- a/chrome/browser/cocoa/browser_window_controller.mm
+++ b/chrome/browser/cocoa/browser_window_controller.mm
@@ -3,7 +3,8 @@
 // found in the LICENSE file.
 
 #import "chrome/app/chrome_dll_resource.h"  // IDC_*
-#import "chrome/browser/browser.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/browser_list.h"
 #import "chrome/browser/cocoa/browser_window_cocoa.h"
 #import "chrome/browser/cocoa/browser_window_controller.h"
 #import "chrome/browser/cocoa/tab_strip_view.h"
@@ -93,6 +94,11 @@
   return YES;
 }
 
+// Called right after our window became the main window.
+- (void)windowDidBecomeMain:(NSNotification *)notification {
+  BrowserList::SetLastActive(browser_);
+}
+
 // Update a toggle state for an NSMenuItem if modified.
 // Take care to insure |item| looks like a NSMenuItem.
 // Called by validateUserInterfaceItem:.
diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp
index 67a0492..920cc51 100644
--- a/chrome/chrome.gyp
+++ b/chrome/chrome.gyp
@@ -495,6 +495,8 @@
         'browser/cocoa/bookmark_bar_state_controller.mm',
         'browser/cocoa/bookmark_menu_bridge.h',
         'browser/cocoa/bookmark_menu_bridge.mm',
+	'browser/cocoa/bookmark_menu_cocoa_controller.h',
+	'browser/cocoa/bookmark_menu_cocoa_controller.mm',
         'browser/cocoa/browser_test_helper.h',
         'browser/cocoa/browser_window_cocoa.h',
         'browser/cocoa/browser_window_cocoa.mm',
@@ -2025,6 +2027,7 @@
         # exclude them from non-Mac builds.
         'browser/cocoa/bookmark_bar_state_controller_unittest.mm',
         'browser/cocoa/bookmark_menu_bridge_unittest.mm',
+        'browser/cocoa/bookmark_menu_cocoa_controller_unittest.mm',
         'browser/command_updater_unittest.cc',
         'browser/download/download_manager_unittest.cc',
         'browser/download/download_request_manager_unittest.cc',