Add favicons to the Mac bookmark menu

Landing CL for rsesek@chromium.org
Original CL: http://codereview.chromium.org/172084



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

5 files changed, 170 insertions, 92 deletions

diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm
index ccec4f9..3920fc6 100644
--- a/chrome/browser/app_controller_mac.mm
+++ b/chrome/browser/app_controller_mac.mm
@@ -263,7 +263,7 @@
   DCHECK(g_browser_process);
   g_browser_process->AddRefModule();
 
-  bookmarkMenuBridge_.reset(new BookmarkMenuBridge());
+  bookmarkMenuBridge_.reset(new BookmarkMenuBridge([self defaultProfile]));
   historyMenuBridge_.reset(new HistoryMenuBridge([self defaultProfile]));
 
   [self setUpdateCheckInterval];
diff --git a/chrome/browser/cocoa/bookmark_menu_bridge.h b/chrome/browser/cocoa/bookmark_menu_bridge.h
index a69ce63..20f7ecc 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge.h
+++ b/chrome/browser/cocoa/bookmark_menu_bridge.h
@@ -20,17 +20,18 @@
 #ifndef CHROME_BROWSER_COCOA_BOOKMARK_MENU_BRIDGE_H_
 #define CHROME_BROWSER_COCOA_BOOKMARK_MENU_BRIDGE_H_
 
+#include <map>
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
-#include "chrome/browser/browser_list.h"
 
-class Browser;
+class BookmarkNode;
+class Profile;
 @class NSMenu;
+@class NSMenuItem;
 @class BookmarkMenuCocoaController;
 
-class BookmarkMenuBridge : public BookmarkModelObserver,
-                           public BrowserList::Observer {
+class BookmarkMenuBridge : public BookmarkModelObserver {
  public:
-  BookmarkMenuBridge();
+  BookmarkMenuBridge(Profile* profile);
   virtual ~BookmarkMenuBridge();
 
   // Overridden from BookmarkModelObserver
@@ -55,14 +56,9 @@ class BookmarkMenuBridge : public BookmarkModelObserver,
   virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
                                              const 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();
+  Profile* GetProfile();
 
  protected:
   // Clear all bookmarks from the given bookmark menu.
@@ -73,8 +69,16 @@ class BookmarkMenuBridge : public BookmarkModelObserver,
   // TODO(jrg): add a counter to enforce maximum nodes added
   void AddNodeToMenu(const BookmarkNode* node, NSMenu* menu);
 
+  // This configures an NSMenuItem with all the data from a BookmarkNode. This
+  // is used to update existing menu items, as well as to configure newly
+  // created ones, like in AddNodeToMenu().
+  void ConfigureMenuItem(const BookmarkNode* node, NSMenuItem* item);
+
+  // Returns the NSMenuItem for a given BookmarkNode.
+  NSMenuItem* MenuItemForNode(const BookmarkNode* node);
+
   // Return the Bookmark menu.
-  NSMenu* BookmarkMenu();
+  virtual NSMenu* BookmarkMenu();
 
   // Start watching the bookmarks for changes.
   void ObserveBookmarkModel();
@@ -82,8 +86,12 @@ class BookmarkMenuBridge : public BookmarkModelObserver,
  private:
   friend class BookmarkMenuBridgeTest;
 
+  Profile* profile_;  // weak
   BookmarkMenuCocoaController* controller_;  // strong
-  bool observing_;  // are we observing the browser list?
+
+  // In order to appropriately update items in the bookmark menu, without
+  // forcing a rebuild, map the model's nodes to menu items.
+  std::map<const BookmarkNode*, NSMenuItem*> bookmark_nodes_;
 };
 
 #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 d37ea02..0bdb728a 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge.mm
+++ b/chrome/browser/cocoa/bookmark_menu_bridge.mm
@@ -11,29 +11,19 @@
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_list.h"
 #import "chrome/browser/cocoa/bookmark_menu_cocoa_controller.h"
+#import "chrome/browser/cocoa/nsimage_cache.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/profile_manager.h"
+#include "skia/ext/skia_utils_mac.h"
 
-BookmarkMenuBridge::BookmarkMenuBridge()
-    : controller_([[BookmarkMenuCocoaController alloc] initWithBridge:this]) {
-  // Depending on when this is constructed, we may or may not have a
-  // browser yet.  If we do, start watching the bookmarks.  If we do
-  // not, delay watching until we see a SetLastActive().  At this time
-  // (6/12/09), the BookmarkMenuBridge object is constructed AFTER the
-  // first SetLastActive in -[AppController applicationDidFinishLaunching:].
-  Browser* browser = BrowserList::GetLastActive();
-  if (browser) {
-    observing_ = false;  // not observing browser activation
+BookmarkMenuBridge::BookmarkMenuBridge(Profile* profile)
+    : profile_(profile),
+      controller_([[BookmarkMenuCocoaController alloc] initWithBridge:this]) {
+  if (GetBookmarkModel())
     ObserveBookmarkModel();
-  } else {
-    observing_ = true;
-    BrowserList::AddObserver(this);
-  }
 }
 
 BookmarkMenuBridge::~BookmarkMenuBridge() {
-  if (observing_)
-    BrowserList::RemoveObserver(this);
   BookmarkModel *model = GetBookmarkModel();
   if (model)
     model->RemoveObserver(this);
@@ -91,15 +81,16 @@ void BookmarkMenuBridge::BookmarkNodeRemoved(BookmarkModel* model,
 
 void BookmarkMenuBridge::BookmarkNodeChanged(BookmarkModel* model,
                                              const BookmarkNode* node) {
-  // TODO(jrg): this is brute force; perhaps we should be nicer.
-  Loaded(model);
+  NSMenuItem* item = MenuItemForNode(node);
+  if (item)
+    ConfigureMenuItem(node, item);
 }
 
 void BookmarkMenuBridge::BookmarkNodeFavIconLoaded(BookmarkModel* model,
                                                    const BookmarkNode* node) {
-  // Nothing to do here -- no icons in the menubar menus yet.
-  // TODO(jrg):
-  // Both Safari and FireFox have icons in their menubars for bookmarks.
+  NSMenuItem* item = MenuItemForNode(node);
+  if (item)
+    ConfigureMenuItem(node, item);
 }
 
 void BookmarkMenuBridge::BookmarkNodeChildrenReordered(
@@ -108,25 +99,6 @@ void BookmarkMenuBridge::BookmarkNodeChildrenReordered(
   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);
-  observing_ = false;
-  ObserveBookmarkModel();
-}
-
 // Watch for changes.
 void BookmarkMenuBridge::ObserveBookmarkModel() {
   BookmarkModel* model = GetBookmarkModel();
@@ -136,20 +108,17 @@ void BookmarkMenuBridge::ObserveBookmarkModel() {
 }
 
 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;
+  if (!profile_)
+    return NULL;
+  return profile_->GetBookmarkModel();
 }
 
-Profile* BookmarkMenuBridge::GetDefaultProfile() {
-  // The delegate of the main application is an AppController
-  return [[NSApp delegate] defaultProfile];
+Profile* BookmarkMenuBridge::GetProfile() {
+  return profile_;
 }
 
 void BookmarkMenuBridge::ClearBookmarkMenu(NSMenu* menu) {
+  bookmark_nodes_.clear();
   // Recursively delete all menus that look like a bookmark.  Assume
   // all items with submenus contain only bookmarks.  This typically
   // deletes everything except the first two items ("Add Bookmark..."
@@ -178,21 +147,50 @@ void BookmarkMenuBridge::AddNodeToMenu(const BookmarkNode* node, NSMenu* menu) {
                                                    action:nil
                                             keyEquivalent:@""] autorelease];
     [menu addItem:item];
+    bookmark_nodes_[child] = item;
     if (child->is_folder()) {
       NSMenu* submenu = [[[NSMenu alloc] initWithTitle:title] autorelease];
       [menu setSubmenu:submenu forItem:item];
       AddNodeToMenu(child, submenu);  // recursive call
     } else {
-      [item setTarget:controller_];
-      [item setAction:@selector(openBookmarkMenuItem:)];
-      [item setTag:child->id()];
-      // Add a tooltip
-      std::string url_string = child->GetURL().possibly_invalid_spec();
-      NSString* tooltip = [NSString stringWithFormat:@"%@\n%s",
-                                    base::SysWideToNSString(child->GetTitle()),
-                                    url_string.c_str()];
-      [item setToolTip:tooltip];
-
+      ConfigureMenuItem(child, item);
     }
   }
 }
+
+void BookmarkMenuBridge::ConfigureMenuItem(const BookmarkNode* node,
+                                           NSMenuItem* item) {
+  [item setTarget:controller_];
+  [item setAction:@selector(openBookmarkMenuItem:)];
+  [item setTag:node->id()];
+  // Add a tooltip
+  std::string url_string = node->GetURL().possibly_invalid_spec();
+  NSString* tooltip = [NSString stringWithFormat:@"%@\n%s",
+                                base::SysWideToNSString(node->GetTitle()),
+                                url_string.c_str()];
+  [item setToolTip:tooltip];
+
+  // Check to see if we have a favicon.
+  NSImage* favicon = nil;
+  BookmarkModel* model = GetBookmarkModel();
+  if (model) {
+    const SkBitmap& bitmap = model->GetFavIcon(node);
+    if (!bitmap.isNull())
+      favicon = gfx::SkBitmapToNSImage(bitmap);
+  }
+  // Either we do not have a loaded favicon or the conversion from SkBitmap
+  // failed. Use the default site image instead.
+  if (!favicon)
+    favicon = nsimage_cache::ImageNamed(@"nav.pdf");
+  [item setImage:favicon];
+}
+
+NSMenuItem* BookmarkMenuBridge::MenuItemForNode(const BookmarkNode* node) {
+  if (!node)
+    return nil;
+  std::map<const BookmarkNode*, NSMenuItem*>::iterator it =
+      bookmark_nodes_.find(node);
+  if (it == bookmark_nodes_.end())
+    return nil;
+  return it->second;
+}
diff --git a/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm b/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
index 813b03c..1d58de7 100644
--- a/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_menu_bridge_unittest.mm
@@ -3,16 +3,40 @@
 // found in the LICENSE file.
 
 #import <AppKit/AppKit.h>
+#import "base/scoped_nsobject.h"
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/cocoa/bookmark_menu_bridge.h"
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+class TestBookmarkMenuBridge : public BookmarkMenuBridge {
+ public:
+  TestBookmarkMenuBridge(Profile* profile)
+      : BookmarkMenuBridge(profile),
+        menu_([[NSMenu alloc] initWithTitle:@"test"]) {
+  }
+  virtual ~TestBookmarkMenuBridge() {}
+
+  scoped_nsobject<NSMenu> menu_;
+
+ protected:
+  // Overridden from BookmarkMenuBridge.
+  virtual NSMenu* BookmarkMenu() {
+    return menu_;
+  }
+};
 
 // TODO(jrg): see refactor comment in bookmark_bar_state_controller_unittest.mm
-class BookmarkMenuBridgeTest : public testing::Test {
+class BookmarkMenuBridgeTest : public PlatformTest {
  public:
 
+   void SetUp() {
+     bridge_.reset(new TestBookmarkMenuBridge(browser_test_helper_.profile()));
+     EXPECT_TRUE(bridge_.get());
+   }
+
   // We are a friend of BookmarkMenuBridge (and have access to
   // protected methods), but none of the classes generated by TEST_F()
   // are.  This (and AddNodeToMenu()) are simple wrappers to let
@@ -26,6 +50,11 @@ class BookmarkMenuBridgeTest : public testing::Test {
     bridge->AddNodeToMenu(root, menu);
   }
 
+  NSMenuItem* MenuItemForNode(BookmarkMenuBridge* bridge,
+                              const BookmarkNode* node) {
+    return bridge->MenuItemForNode(node);
+  }
+
   NSMenuItem* AddItemToMenu(NSMenu *menu, NSString *title, SEL selector) {
     NSMenuItem *item = [[[NSMenuItem alloc] initWithTitle:title action:NULL
                                             keyEquivalent:@""] autorelease];
@@ -36,15 +65,13 @@ class BookmarkMenuBridgeTest : public testing::Test {
   }
 
   BrowserTestHelper browser_test_helper_;
+  scoped_ptr<TestBookmarkMenuBridge> bridge_;
 };
 
 
 // Test that ClearBookmarkMenu() removes all bookmark menus.
 TEST_F(BookmarkMenuBridgeTest, TestClearBookmarkMenu) {
-  scoped_ptr<BookmarkMenuBridge> bridge(new BookmarkMenuBridge());
-  EXPECT_TRUE(bridge.get());
-
-  NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"foo"] autorelease];
+  NSMenu* menu = bridge_->menu_.get();
 
   AddItemToMenu(menu, @"hi mom", nil);
   AddItemToMenu(menu, @"not", @selector(openBookmarkMenuItem:));
@@ -53,7 +80,7 @@ TEST_F(BookmarkMenuBridgeTest, TestClearBookmarkMenu) {
   AddItemToMenu(menu, @"not", @selector(openBookmarkMenuItem:));
   AddItemToMenu(menu, @"zippy", @selector(length));
 
-  ClearBookmarkMenu(bridge.get(), menu);
+  ClearBookmarkMenu(bridge_.get(), menu);
 
   // Make sure all bookmark items are removed, and all items with
   // submenus removed.
@@ -67,16 +94,10 @@ TEST_F(BookmarkMenuBridgeTest, TestClearBookmarkMenu) {
 // including the recursive case.
 TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
   std::wstring empty;
-  Profile* profile = browser_test_helper_.profile();
-
-  scoped_ptr<BookmarkMenuBridge> bridge(new BookmarkMenuBridge());
-  EXPECT_TRUE(bridge.get());
+  NSMenu* menu = bridge_->menu_.get();
 
-  NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"foo"] autorelease];
-
-  BookmarkModel* model = profile->GetBookmarkModel();
-  const BookmarkNode* bookmark_bar = model->GetBookmarkBarNode();
-  const BookmarkNode* root = model->AddGroup(bookmark_bar, 0, empty);
+  BookmarkModel* model = bridge_->GetBookmarkModel();
+  const BookmarkNode* root = model->GetBookmarkBarNode();
   EXPECT_TRUE(model && root);
 
   const char* short_url = "http://foo/";
@@ -95,9 +116,6 @@ TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
   // And the submenu fo the middle one
   model->AddURL(node, 0, empty, GURL("http://sub"));
 
-  // Add to the NSMenu, then confirm it looks good
-  AddNodeToMenu(bridge.get(), root, menu);
-
   EXPECT_EQ(3, [menu numberOfItems]);
 
   // Confirm for just the first and last (index 0 and 2)
@@ -129,4 +147,58 @@ TEST_F(BookmarkMenuBridgeTest, TestAddNodeToMenu) {
   // e.g. http://foo becomes http://foo/)
   EXPECT_GE([[[menu itemAtIndex:0] toolTip] length], (2*strlen(short_url) - 5));
   EXPECT_GE([[[menu itemAtIndex:2] toolTip] length], (2*strlen(long_url) - 5));
+
+  // Make sure the favicon is non-nil (should be either the default site
+  // icon or a favicon, if present).
+  EXPECT_TRUE([[menu itemAtIndex:0] image]);
+  EXPECT_TRUE([[menu itemAtIndex:2] image]);
+}
+
+// Makes sure our internal map of BookmarkNode to NSMenuItem works.
+TEST_F(BookmarkMenuBridgeTest, TestGetMenuItemForNode) {
+  std::wstring empty;
+  NSMenu* menu = bridge_->menu_.get();
+
+  BookmarkModel* model = bridge_->GetBookmarkModel();
+  const BookmarkNode* bookmark_bar = model->GetBookmarkBarNode();
+  const BookmarkNode* root = model->AddGroup(bookmark_bar, 0, empty);
+  EXPECT_TRUE(model && root);
+
+  model->AddURL(root, 0, ASCIIToWide("Test Item"), GURL("http://test"));
+  AddNodeToMenu(bridge_.get(), root, menu);
+  EXPECT_TRUE(MenuItemForNode(bridge_.get(), root->GetChild(0)));
+
+  model->AddURL(root, 1, ASCIIToWide("Test 2"), GURL("http://second-test"));
+  AddNodeToMenu(bridge_.get(), root, menu);
+  EXPECT_TRUE(MenuItemForNode(bridge_.get(), root->GetChild(0)));
+  EXPECT_TRUE(MenuItemForNode(bridge_.get(), root->GetChild(1)));
+
+  const BookmarkNode* removed_node = root->GetChild(0);
+  EXPECT_EQ(2, root->GetChildCount());
+  model->Remove(root, 0);
+  EXPECT_EQ(1, root->GetChildCount());
+  EXPECT_FALSE(MenuItemForNode(bridge_.get(), removed_node));
+  EXPECT_TRUE(MenuItemForNode(bridge_.get(), root->GetChild(0)));
+
+  const BookmarkNode empty_node(GURL("http://no-where/"));
+  EXPECT_FALSE(MenuItemForNode(bridge_.get(), &empty_node));
+  EXPECT_FALSE(MenuItemForNode(bridge_.get(), NULL));
+}
+
+TEST_F(BookmarkMenuBridgeTest, TestFavIconLoading) {
+  std::wstring empty;
+  NSMenu* menu = bridge_->menu_;
+
+  BookmarkModel* model = bridge_->GetBookmarkModel();
+  const BookmarkNode* root = model->GetBookmarkBarNode();
+  EXPECT_TRUE(model && root);
+
+  const BookmarkNode* node =
+      model->AddURL(root, 0, ASCIIToWide("Test Item"),
+                    GURL("http://favicon-test"));
+  NSMenuItem* item = [menu itemAtIndex:0];
+  EXPECT_TRUE([item image]);
+  [item setImage:nil];
+  bridge_->BookmarkNodeFavIconLoaded(model, node);
+  EXPECT_TRUE([item image]);
 }
diff --git a/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm b/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm
index d3acf3a..f053da7 100644
--- a/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm
+++ b/chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm
@@ -51,7 +51,7 @@ const NSUInteger kMaximumMenuPixelsWide = 300;
 // Open the URL of the given BookmarkNode in the current tab.
 - (void)openURLForNode:(const BookmarkNode*)node {
   Browser* browser =
-      Browser::GetOrCreateTabbedBrowser(bridge_->GetDefaultProfile());
+      Browser::GetOrCreateTabbedBrowser(bridge_->GetProfile());
   WindowOpenDisposition disposition =
       event_utils::WindowOpenDispositionFromNSEvent([NSApp currentEvent]);
   browser->OpenURL(node->GetURL(), GURL(), disposition,