[Mac] Add favicons to the history menu

BUG=20464
TEST=Open History menu, see icons.

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

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

6 files changed, 323 insertions, 104 deletions

diff --git a/chrome/browser/cocoa/history_menu_bridge.h b/chrome/browser/cocoa/history_menu_bridge.h
index c67ba83..488909f 100644
--- a/chrome/browser/cocoa/history_menu_bridge.h
+++ b/chrome/browser/cocoa/history_menu_bridge.h
@@ -1,32 +1,16 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
-// C++ controller for the history menu; one per AppController (means there
-// is only one). This class observes various data sources, namely the
-// HistoryService and the TabRestoreService, and then updates the NSMenu when
-// there is new data.
-//
-// The history menu is broken up into sections: most visisted and recently
-// closed. The overall menu has a tag of IDC_HISTORY_MENU, with two sections
-// IDC_HISTORY_MENU_VISITED and IDC_HISTORY_MENU_CLOSED, which are used to
-// delineate the two sections. Items within these sections are assigned tags
-// within IDC_HISTORY_MENU_* + 1..99. 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 HistoryMenuCocoaController, not
-// firstResponder. See HistoryMenuBridge::AddItemToMenu()). Unlike most of our
-// Cocoa-Bridge classes, the HMCC is not at the root of the ownership model
-// because its only function is to respond to menu item actions; everything
-// else is done in this bridge.
-
 #ifndef CHROME_BROWSER_COCOA_HISTORY_MENU_BRIDGE_H_
 #define CHROME_BROWSER_COCOA_HISTORY_MENU_BRIDGE_H_
 
 #import <Cocoa/Cocoa.h>
 #include "base/scoped_nsobject.h"
+#include "base/scoped_ptr.h"
+#include "base/scoped_vector.h"
 #include "chrome/browser/cancelable_request.h"
+#import "chrome/browser/favicon_service.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/sessions/tab_restore_service.h"
 #include "chrome/common/notification_observer.h"
@@ -38,20 +22,56 @@ class Profile;
 class TabNavigationEntry;
 @class HistoryMenuCocoaController;
 
+// C++ controller for the history menu; one per AppController (means there
+// is only one). This class observes various data sources, namely the
+// HistoryService and the TabRestoreService, and then updates the NSMenu when
+// there is new data.
+//
+// The history menu is broken up into sections: most visisted and recently
+// closed. The overall menu has a tag of IDC_HISTORY_MENU, with two sections
+// IDC_HISTORY_MENU_VISITED and IDC_HISTORY_MENU_CLOSED, which are used to
+// delineate the two sections. Items within these sections are assigned tags
+// within IDC_HISTORY_MENU_* + 1..99. 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, history menu items are created
+// dynamically. They also have a target of HistoryMenuCocoaController, not
+// firstResponder. See HistoryMenuBridge::AddItemToMenu(). Unlike most of our
+// Cocoa-Bridge classes, the HMCC is not at the root of the ownership model
+// because its only function is to respond to menu item actions; everything
+// else is done in this bridge.
 class HistoryMenuBridge : public NotificationObserver,
                           public TabRestoreService::Observer {
  public:
   // This is a generalization of the data we store in the history menu because
   // we pull things from different sources with different data types.
   struct HistoryItem {
-    HistoryItem() {}
+   public:
+    HistoryItem() : icon_requested(false) {}
     ~HistoryItem() {}
 
+    // The title for the menu item.
     string16 title;
+    // The URL that will be navigated to if the user selects this item.
     GURL url;
+    // Favicon for the URL.
+    scoped_nsobject<NSImage> icon;
+
+    // If the icon is being requested from the FaviconService, |icon_requested|
+    // will be true and |icon_handle| will be non-NULL. If this is false, then
+    // |icon_handle| will be NULL.
+    bool icon_requested;
+    // The Handle given to us by the FaviconService for the icon fetch request.
+    FaviconService::Handle icon_handle;
+
+    // The pointer to the item after it has been created. Weak.
+    NSMenuItem* menu_item;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(HistoryItem);
   };
 
-  HistoryMenuBridge(Profile* profile);
+  explicit HistoryMenuBridge(Profile* profile);
   virtual ~HistoryMenuBridge();
 
   // Overriden from NotificationObserver.
@@ -67,8 +87,8 @@ class HistoryMenuBridge : public NotificationObserver,
   // to access model information when responding to actions.
   HistoryService* service();
   Profile* profile();
-  std::vector<HistoryItem>* visited_results();
-  std::vector<HistoryItem>* closed_results();
+  const ScopedVector<HistoryItem>* const visited_results();
+  const ScopedVector<HistoryItem>* const closed_results();
 
  protected:
   // Return the History menu.
@@ -80,7 +100,7 @@ class HistoryMenuBridge : public NotificationObserver,
   void ClearMenuSection(NSMenu* menu, NSInteger tag, unsigned int count);
 
   // Adds a given title and URL to HistoryMenu() with a certain tag and index.
-  void AddItemToMenu(const HistoryItem& item,
+  void AddItemToMenu(HistoryItem* item,
                      NSMenu* menu,
                      NSInteger tag,
                      NSInteger index);
@@ -102,6 +122,24 @@ class HistoryMenuBridge : public NotificationObserver,
   // to |closed_results_|. Return TRUE if the operation completed successfully.
   bool AddNavigationForTab(const TabRestoreService::Tab& entry);
 
+  // Helper function that sends an async request to the FaviconService to get
+  // an icon. The callback will update the NSMenuItem directly.
+  void GetFaviconForHistoryItem(HistoryItem* item);
+
+  // Callback for the FaviconService to return favicon image data when we
+  // request it. This decodes the raw data, updates the HistoryItem, and then
+  // sets the image on the menu. Called on the same same thread that
+  // GetFaviconForHistoryItem() was called on (UI thread).
+  void GotFaviconData(FaviconService::Handle handle,
+                      bool know_favicon,
+                      scoped_refptr<RefCountedBytes> data,
+                      bool expired,
+                      GURL url);
+
+  // Cancels a favicon load request for a given HistoryItem, if one is in
+  // progress.
+  void CancelFaviconRequest(HistoryItem* item);
+
  private:
   friend class HistoryMenuBridgeTest;
 
@@ -115,8 +153,11 @@ class HistoryMenuBridge : public NotificationObserver,
   CancelableRequestConsumer cancelable_request_consumer_;
 
   // The most recent results we've received.
-  std::vector<HistoryItem> visited_results_;
-  std::vector<HistoryItem> closed_results_;
+  ScopedVector<HistoryItem> visited_results_;
+  ScopedVector<HistoryItem> closed_results_;
+
+  // Maps HistoryItems to favicon request Handles.
+  CancelableRequestConsumerTSimple<HistoryItem*> favicon_consumer_;
 
   // We coalesce requests to re-create the menu. |create_in_progress_| is true
   // whenever we are either waiting for the history service to return query
@@ -125,6 +166,9 @@ class HistoryMenuBridge : public NotificationObserver,
   bool create_in_progress_;
   bool need_recreate_;
 
+  // The default favicon if a HistoryItem does not have one.
+  scoped_nsobject<NSImage> default_favicon_;
+
   DISALLOW_COPY_AND_ASSIGN(HistoryMenuBridge);
 };
 
diff --git a/chrome/browser/cocoa/history_menu_bridge.mm b/chrome/browser/cocoa/history_menu_bridge.mm
index 30b4488..0cc64b6 100644
--- a/chrome/browser/cocoa/history_menu_bridge.mm
+++ b/chrome/browser/cocoa/history_menu_bridge.mm
@@ -3,9 +3,13 @@
 // found in the LICENSE file.
 
 #include "chrome/browser/cocoa/history_menu_bridge.h"
+
+#include "app/gfx/codec/png_codec.h"
+#include "app/resource_bundle.h"
 #include "base/callback.h"
-#include "base/sys_string_conversions.h"
+#include "base/stl_util-inl.h"
 #include "base/string_util.h"
+#include "base/sys_string_conversions.h"
 #include "chrome/app/chrome_dll_resource.h"  // IDC_HISTORY_MENU
 #import "chrome/browser/app_controller_mac.h"
 #import "chrome/browser/cocoa/history_menu_cocoa_controller.h"
@@ -15,6 +19,8 @@
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/url_constants.h"
+#include "grit/app_resources.h"
+#include "skia/ext/skia_utils_mac.h"
 
 namespace {
 
@@ -60,6 +66,9 @@ HistoryMenuBridge::HistoryMenuBridge(Profile* profile)
       tab_restore_service_->AddObserver(this);
   }
 
+  ResourceBundle& rb = ResourceBundle::GetSharedInstance();
+  default_favicon_.reset([rb.GetNSImageNamed(IDR_DEFAULT_FAVICON) retain]);
+
   // The service is not ready for use yet, so become notified when it does.
   if (!history_service_) {
     registrar_.Add(this,
@@ -81,6 +90,15 @@ HistoryMenuBridge::~HistoryMenuBridge() {
 
   if (tab_restore_service_)
     tab_restore_service_->RemoveObserver(this);
+
+  // Cancel any favicon requests.
+  ScopedVector<HistoryItem>::iterator it;
+  for (it = visited_results_.begin(); it != visited_results_.end(); ++it) {
+    CancelFaviconRequest(*it);
+  }
+  for (it = closed_results_.begin(); it != closed_results_.end(); ++it) {
+    CancelFaviconRequest(*it);
+  }
 }
 
 void HistoryMenuBridge::Observe(NotificationType type,
@@ -119,7 +137,7 @@ void HistoryMenuBridge::TabRestoreServiceChanged(TabRestoreService* service) {
   // what's cached, throw it away and rebuild it. It probably means
   // the browsing data was cleared by the user.
   if (entries.size() < closed_results_.size())
-    closed_results_.clear();
+    closed_results_.reset();
 
   unsigned int added_count = 0;
   for (TabRestoreService::Entries::const_iterator it = entries.begin();
@@ -148,15 +166,15 @@ void HistoryMenuBridge::TabRestoreServiceChanged(TabRestoreService* service) {
 
   // Remove extraneous/old results.
   if (closed_results_.size() > kRecentlyClosedCount)
-    closed_results_.erase(closed_results_.begin(),
-                          closed_results_.end() - kRecentlyClosedCount);
+    STLDeleteContainerPointers(closed_results_.begin(),
+        closed_results_.end() - kRecentlyClosedCount);
 
   NSInteger top_index = [menu indexOfItemWithTag:IDC_HISTORY_MENU_CLOSED] + 1;
 
   int i = 0;  // Count offsets for |tag| and |index| in AddItemToMenu().
-  for (std::vector<HistoryItem>::const_iterator it = closed_results_.begin();
+  for (ScopedVector<HistoryItem>::const_iterator it = closed_results_.begin();
       it != closed_results_.end(); ++it) {
-    HistoryItem item = *it;
+    HistoryItem* item = *it;
     NSInteger tag = IDC_HISTORY_MENU_CLOSED + 1 + i;
     AddItemToMenu(item, HistoryMenu(), tag, top_index + i);
     ++i;
@@ -176,12 +194,12 @@ Profile* HistoryMenuBridge::profile() {
   return profile_;
 }
 
-std::vector<HistoryMenuBridge::HistoryItem>*
+const ScopedVector<HistoryMenuBridge::HistoryItem>* const
     HistoryMenuBridge::visited_results() {
   return &visited_results_;
 }
 
-std::vector<HistoryMenuBridge::HistoryItem>*
+const ScopedVector<HistoryMenuBridge::HistoryItem>* const
     HistoryMenuBridge::closed_results() {
   return &closed_results_;
 }
@@ -210,6 +228,14 @@ void HistoryMenuBridge::ClearMenuSection(NSMenu* menu,
     NSMenuItem* menu_item = [menu itemAtIndex:index];
     item_tag = [menu_item tag];
     if ([menu_item action] == @selector(openHistoryMenuItem:)) {
+      // If there is a pending favicon request for this menu item, find and
+      // cancel it.
+      HistoryItem* item =
+          const_cast<HistoryItem*>([controller_ itemForTag:item_tag]);
+      if (item)
+        CancelFaviconRequest(item);
+
+      // Now remove it from the menu.
       [menu removeItemAtIndex:index];
       --count;
     }
@@ -219,12 +245,12 @@ void HistoryMenuBridge::ClearMenuSection(NSMenu* menu,
   }
 }
 
-void HistoryMenuBridge::AddItemToMenu(const HistoryItem& item,
+void HistoryMenuBridge::AddItemToMenu(HistoryItem* item,
                                       NSMenu* menu,
                                       NSInteger tag,
                                       NSInteger index) {
-  NSString* title = base::SysUTF16ToNSString(item.title);
-  std::string url_string = item.url.possibly_invalid_spec();
+  NSString* title = base::SysUTF16ToNSString(item->title);
+  std::string url_string = item->url.possibly_invalid_spec();
 
   // If we don't have a title, use the URL.
   if ([title isEqualToString:@""])
@@ -245,6 +271,10 @@ void HistoryMenuBridge::AddItemToMenu(const HistoryItem& item,
   [menu_item setTarget:controller_];
   [menu_item setAction:@selector(openHistoryMenuItem:)];
   [menu_item setTag:tag];
+  if (item->icon.get())
+    [menu_item setImage:item->icon.get()];
+  else
+    [menu_item setImage:default_favicon_.get()];
 
   // Add a tooltip.
   NSString* tooltip = [NSString stringWithFormat:@"%@\n%s", full_title,
@@ -252,6 +282,7 @@ void HistoryMenuBridge::AddItemToMenu(const HistoryItem& item,
   [menu_item setToolTip:tooltip];
 
   [menu insertItem:menu_item atIndex:index];
+  item->menu_item = menu_item;
 }
 
 void HistoryMenuBridge::Init() {
@@ -282,16 +313,22 @@ void HistoryMenuBridge::OnVisitedHistoryResults(
   NSInteger top_item = [menu indexOfItemWithTag:IDC_HISTORY_MENU_VISITED] + 1;
 
   ClearMenuSection(menu, IDC_HISTORY_MENU_VISITED, visited_results_.size());
-  visited_results_.clear();
+  visited_results_.reset();
 
   size_t count = results->size();
   for (size_t i = 0; i < count; ++i) {
     PageUsageData* history_item = (*results)[i];
 
-    HistoryItem item;
-    item.title = history_item->GetTitle();
-    item.url = history_item->GetURL();
-    visited_results_.push_back(item);
+    HistoryItem* item = new HistoryItem();
+    item->title = history_item->GetTitle();
+    item->url = history_item->GetURL();
+    if (history_item->HasFavIcon()) {
+      const SkBitmap* icon = history_item->GetFavIcon();
+      item->icon.reset([gfx::SkBitmapToNSImage(*icon) retain]);
+    } else {
+      GetFaviconForHistoryItem(item);
+    }
+    visited_results_.push_back(item);  // ScopedVector takes ownership.
 
     // Use the large gaps in tags assignment to create the tag for history menu
     // items.
@@ -316,9 +353,62 @@ bool HistoryMenuBridge::AddNavigationForTab(
   if (current_navigation.url() == GURL(chrome::kChromeUINewTabURL))
     return false;
 
-  HistoryItem item;
-  item.title = current_navigation.title();
-  item.url = current_navigation.url();
-  closed_results_.push_back(item);
+  HistoryItem* item = new HistoryItem();
+  item->title = current_navigation.title();
+  item->url = current_navigation.url();
+  closed_results_.push_back(item);  // ScopedVector takes ownership.
+
+  // Tab navigations don't come with icons, so we always have to request them.
+  GetFaviconForHistoryItem(item);
+
   return true;
 }
+
+void HistoryMenuBridge::GetFaviconForHistoryItem(HistoryItem* item) {
+  FaviconService* service =
+      profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
+  FaviconService::Handle handle = service->GetFaviconForURL(item->url,
+      &favicon_consumer_,
+      NewCallback(this, &HistoryMenuBridge::GotFaviconData));
+  favicon_consumer_.SetClientData(service, handle, item);
+  item->icon_handle = handle;
+  item->icon_requested = true;
+}
+
+void HistoryMenuBridge::GotFaviconData(FaviconService::Handle handle,
+                                       bool know_favicon,
+                                       scoped_refptr<RefCountedBytes> data,
+                                       bool expired,
+                                       GURL url) {
+  // Since we're going to do Cocoa-y things, make sure this is the main thread.
+  DCHECK([NSThread isMainThread]);
+
+  HistoryItem* item =
+      favicon_consumer_.GetClientData(
+          profile_->GetFaviconService(Profile::EXPLICIT_ACCESS), handle);
+  item->icon_requested = false;
+  item->icon_handle = NULL;
+
+  // Convert the raw data to Skia and then to a NSImage.
+  // TODO(rsesek): Is there an easier way to do this?
+  SkBitmap icon;
+  if (know_favicon && data.get() && data->size() &&
+      gfx::PNGCodec::Decode(data->front(), data->size(), &icon)) {
+    NSImage* image = gfx::SkBitmapToNSImage(icon);
+    if (image) {
+      // The conversion was successful.
+      item->icon.reset([image retain]);
+      [item->menu_item setImage:item->icon.get()];
+    }
+  }
+}
+
+void HistoryMenuBridge::CancelFaviconRequest(HistoryItem* item) {
+  if (item->icon_requested) {
+    FaviconService* service =
+        profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
+    service->CancelRequest(item->icon_handle);
+    item->icon_requested = false;
+    item->icon_handle = NULL;
+  }
+}
diff --git a/chrome/browser/cocoa/history_menu_bridge_unittest.mm b/chrome/browser/cocoa/history_menu_bridge_unittest.mm
index a0e45e6..84efb4f 100644
--- a/chrome/browser/cocoa/history_menu_bridge_unittest.mm
+++ b/chrome/browser/cocoa/history_menu_bridge_unittest.mm
@@ -1,19 +1,29 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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 <vector>
+
+#include "app/gfx/codec/png_codec.h"
+#include "base/ref_counted_memory.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/browser.h"
+#include "chrome/browser/cancelable_request.h"
+#include "chrome/browser/cocoa/cocoa_test_helper.h"
 #include "chrome/browser/cocoa/history_menu_bridge.h"
 #include "chrome/browser/cocoa/browser_test_helper.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "testing/platform_test.h"
 
-class HistoryMenuBridgeTest : public PlatformTest {
+class HistoryMenuBridgeTest : public CocoaTest {
  public:
 
+  virtual void SetUp() {
+    CocoaTest::SetUp();
+    browser_test_helper_.profile()->CreateFaviconService();
+  }
+
   // We are a friend of HistoryMenuBridge (and have access to
   // protected methods), but none of the classes generated by TEST_F()
   // are. Wraps common commands.
@@ -25,7 +35,7 @@ class HistoryMenuBridgeTest : public PlatformTest {
   }
 
   void AddItemToBridgeMenu(HistoryMenuBridge* bridge,
-                           HistoryMenuBridge::HistoryItem& item,
+                           HistoryMenuBridge::HistoryItem* item,
                            NSMenu* menu,
                            NSInteger tag,
                            NSInteger index) {
@@ -45,46 +55,65 @@ class HistoryMenuBridgeTest : public PlatformTest {
     return item;
   }
 
-  HistoryMenuBridge::HistoryItem CreateItem(const string16& title) {
-    HistoryMenuBridge::HistoryItem item;
-    item.title = title;
-    item.url = GURL("http://google.com");
+  HistoryMenuBridge::HistoryItem* CreateItem(const string16& title) {
+    HistoryMenuBridge::HistoryItem* item =
+        new HistoryMenuBridge::HistoryItem();
+    item->title = title;
+    item->url = GURL(title);
     return item;
   }
 
+  void GetFaviconForHistoryItem(HistoryMenuBridge* bridge,
+                                HistoryMenuBridge::HistoryItem* item) {
+    bridge->GetFaviconForHistoryItem(item);
+  }
+
+  void GotFaviconData(HistoryMenuBridge* bridge,
+                      FaviconService::Handle handle,
+                      bool know_favicon,
+                      scoped_refptr<RefCountedBytes> data,
+                      bool expired,
+                      GURL url) {
+    bridge->GotFaviconData(handle, know_favicon, data, expired, url);
+  }
+
+  CancelableRequestConsumerTSimple<HistoryMenuBridge::HistoryItem*>&
+      favicon_consumer(HistoryMenuBridge* bridge) {
+    return bridge->favicon_consumer_;
+  }
+
   BrowserTestHelper browser_test_helper_;
 };
 
 // Edge case test for clearing until the end of a menu.
-TEST_F(HistoryMenuBridgeTest, TestClearHistoryMenuUntilEnd) {
+TEST_F(HistoryMenuBridgeTest, ClearHistoryMenuUntilEnd) {
   scoped_ptr<HistoryMenuBridge> bridge(
       new HistoryMenuBridge(browser_test_helper_.profile()));
   EXPECT_TRUE(bridge.get());
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease];
-  NSInteger section_tag = 9990;
-  AddItemToMenu(menu, @"HEADER", NULL, section_tag);
-  NSInteger tag = section_tag;
+  NSInteger tag = IDC_HISTORY_MENU_VISITED;
+  AddItemToMenu(menu, @"HEADER", NULL, tag);
 
   AddItemToMenu(menu, @"alpha", @selector(openHistoryMenuItem:), ++tag);
   AddItemToMenu(menu, @"bravo", @selector(openHistoryMenuItem:), ++tag);
   AddItemToMenu(menu, @"charlie", @selector(openHistoryMenuItem:), ++tag);
   AddItemToMenu(menu, @"delta", @selector(openHistoryMenuItem:), ++tag);
 
-  ClearMenuSection(bridge.get(), menu, section_tag, 4);
+  ClearMenuSection(bridge.get(), menu, IDC_HISTORY_MENU_VISITED, 4);
 
   EXPECT_EQ(1, [menu numberOfItems]);
-  EXPECT_EQ(@"HEADER", [[menu itemWithTag:section_tag] title]);
+  EXPECT_EQ(@"HEADER", [[menu itemWithTag:IDC_HISTORY_MENU_VISITED] title]);
 }
 
 // Skip menu items that are not hooked up to |-openHistoryMenuItem:|.
-TEST_F(HistoryMenuBridgeTest, TestClearHistoryMenuSkipping) {
+TEST_F(HistoryMenuBridgeTest, ClearHistoryMenuSkipping) {
   scoped_ptr<HistoryMenuBridge> bridge(
       new HistoryMenuBridge(browser_test_helper_.profile()));
   EXPECT_TRUE(bridge.get());
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease];
-  NSInteger section_tag = 9990;
+  NSInteger section_tag = IDC_HISTORY_MENU_VISITED;
   AddItemToMenu(menu, @"HEADER", NULL, section_tag);
   NSInteger tag = section_tag;
 
@@ -101,44 +130,39 @@ TEST_F(HistoryMenuBridgeTest, TestClearHistoryMenuSkipping) {
 }
 
 // Edge case test for clearing an empty menu.
-TEST_F(HistoryMenuBridgeTest, TestClearHistoryMenuEmpty) {
+TEST_F(HistoryMenuBridgeTest, ClearHistoryMenuEmpty) {
   scoped_ptr<HistoryMenuBridge> bridge(
       new HistoryMenuBridge(browser_test_helper_.profile()));
   EXPECT_TRUE(bridge.get());
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease];
-  NSInteger section_tag = 9990;
-  AddItemToMenu(menu, @"HEADER", NULL, section_tag);
+  AddItemToMenu(menu, @"HEADER", NULL, IDC_HISTORY_MENU_VISITED);
 
-  ClearMenuSection(bridge.get(), menu, section_tag, 1);
+  ClearMenuSection(bridge.get(), menu, IDC_HISTORY_MENU_VISITED, 1);
 
   EXPECT_EQ(1, [menu numberOfItems]);
-  EXPECT_EQ(@"HEADER", [[menu itemWithTag:section_tag] title]);
+  EXPECT_EQ(@"HEADER", [[menu itemWithTag:IDC_HISTORY_MENU_VISITED] title]);
 }
 
 // Test that AddItemToMenu() properly adds HistoryItem objects as menus.
-TEST_F(HistoryMenuBridgeTest, TestAddItemToMenu) {
+TEST_F(HistoryMenuBridgeTest, AddItemToMenu) {
   scoped_ptr<HistoryMenuBridge> bridge(
       new HistoryMenuBridge(browser_test_helper_.profile()));
   EXPECT_TRUE(bridge.get());
 
   NSMenu* menu = [[[NSMenu alloc] initWithTitle:@"history foo"] autorelease];
 
-  string16 short_url = ASCIIToUTF16("http://foo/");
-  string16 long_url = ASCIIToUTF16("http://super-duper-long-url--."
-    "that.cannot.possibly.fit.even-in-80-columns"
-    "or.be.reasonably-displayed-in-a-menu"
-    "without.looking-ridiculous.com/"); // 140 chars total
+  const string16 short_url = ASCIIToUTF16("http://foo/");
+  const string16 long_url = ASCIIToUTF16("http://super-duper-long-url--."
+      "that.cannot.possibly.fit.even-in-80-columns"
+      "or.be.reasonably-displayed-in-a-menu"
+      "without.looking-ridiculous.com/"); // 140 chars total
 
-  HistoryMenuBridge::HistoryItem item1;
-  item1.title = short_url;
-  item1.url = GURL(short_url);
-  AddItemToBridgeMenu(bridge.get(), item1, menu, 100, 0);
+  scoped_ptr<HistoryMenuBridge::HistoryItem> item1(CreateItem(short_url));
+  AddItemToBridgeMenu(bridge.get(), item1.get(), menu, 100, 0);
 
-  HistoryMenuBridge::HistoryItem item2;
-  item2.title = long_url;
-  item2.url = GURL(long_url);
-  AddItemToBridgeMenu(bridge.get(), item2, menu, 101, 1);
+  scoped_ptr<HistoryMenuBridge::HistoryItem> item2(CreateItem(long_url));
+  AddItemToBridgeMenu(bridge.get(), item2.get(), menu, 101, 1);
 
   EXPECT_EQ(2, [menu numberOfItems]);
 
@@ -162,3 +186,59 @@ TEST_F(HistoryMenuBridgeTest, TestAddItemToMenu) {
   EXPECT_GE([[[menu itemAtIndex:0] toolTip] length], (2*short_url.length()-5));
   EXPECT_GE([[[menu itemAtIndex:1] toolTip] length], (2*long_url.length()-5));
 }
+
+// Tests that we properly request an icon from the FaviconService.
+TEST_F(HistoryMenuBridgeTest, GetFaviconForHistoryItem) {
+  TestingProfile* profile = browser_test_helper_.profile();
+  HistoryMenuBridge bridge(profile);
+
+  // Create a fake item.
+  HistoryMenuBridge::HistoryItem item;
+  item.title = ASCIIToUTF16("Title");
+  item.url = GURL("http://google.com");
+
+  // Request the icon.
+  GetFaviconForHistoryItem(&bridge, &item);
+
+  // Make sure that there is ClientData for the request.
+  std::vector<HistoryMenuBridge::HistoryItem*> data;
+  favicon_consumer(&bridge).GetAllClientData(&data);
+  ASSERT_EQ(data.size(), 1U);
+  EXPECT_EQ(&item, data[0]);
+
+  // Make sure the item was modified properly.
+  EXPECT_TRUE(item.icon_requested);
+  EXPECT_GT(item.icon_handle, 0);
+}
+
+TEST_F(HistoryMenuBridgeTest, GotFaviconData) {
+  TestingProfile* profile = browser_test_helper_.profile();
+  HistoryMenuBridge bridge(profile);
+
+  // Create a dummy bitmap.
+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config, 25, 25);
+  bitmap.allocPixels();
+  bitmap.eraseRGB(255, 0, 0);
+
+  // Convert it to raw PNG bytes. We totally ignore color order here because
+  // we just want to test the roundtrip through the Bridge, not that we can
+  // make icons look pretty.
+  std::vector<unsigned char> raw;
+  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, true, &raw);
+  scoped_refptr<RefCountedBytes> bytes(new RefCountedBytes(raw));
+
+  // Set up the HistoryItem.
+  HistoryMenuBridge::HistoryItem item;
+  scoped_nsobject<NSMenuItem> menu_item([[NSMenuItem alloc] init]);
+  item.menu_item = menu_item.get();
+  GetFaviconForHistoryItem(&bridge, &item);
+
+  // Pretend to be called back.
+  GotFaviconData(&bridge, item.icon_handle, true, bytes, false, GURL());
+
+  // Make sure the callback works.
+  EXPECT_EQ(false, item.icon_requested);
+  EXPECT_TRUE(item.icon.get());
+  EXPECT_TRUE([item.menu_item image]);
+}
diff --git a/chrome/browser/cocoa/history_menu_cocoa_controller.h b/chrome/browser/cocoa/history_menu_cocoa_controller.h
index 32a7939..beb5576 100644
--- a/chrome/browser/cocoa/history_menu_cocoa_controller.h
+++ b/chrome/browser/cocoa/history_menu_cocoa_controller.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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.
 
@@ -25,8 +25,8 @@
 @end  // HistoryMenuCocoaController
 
 @interface HistoryMenuCocoaController (ExposedForUnitTests)
-- (HistoryMenuBridge::HistoryItem)itemForTag:(NSInteger)tag;
-- (void)openURLForItem:(HistoryMenuBridge::HistoryItem&)node;
+- (const HistoryMenuBridge::HistoryItem*)itemForTag:(NSInteger)tag;
+- (void)openURLForItem:(const HistoryMenuBridge::HistoryItem*)node;
 @end  // HistoryMenuCocoaController (ExposedForUnitTests)
 
 #endif  // CHROME_BROWSER_COCOA_HISTORY_MENU_COCOA_CONTROLLER_H_
diff --git a/chrome/browser/cocoa/history_menu_cocoa_controller.mm b/chrome/browser/cocoa/history_menu_cocoa_controller.mm
index 89dd2f8..a0672b3 100644
--- a/chrome/browser/cocoa/history_menu_cocoa_controller.mm
+++ b/chrome/browser/cocoa/history_menu_cocoa_controller.mm
@@ -1,7 +1,8 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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 "base/scoped_vector.h"
 #include "chrome/app/chrome_dll_resource.h"  // IDC_HISTORY_MENU
 #include "chrome/browser/browser.h"
 #import "chrome/browser/cocoa/history_menu_cocoa_controller.h"
@@ -24,16 +25,16 @@
 }
 
 // Open the URL of the given history item in the current tab.
-- (void)openURLForItem:(HistoryMenuBridge::HistoryItem&)node {
+- (void)openURLForItem:(const HistoryMenuBridge::HistoryItem*)node {
   Browser* browser = Browser::GetOrCreateTabbedBrowser(bridge_->profile());
   WindowOpenDisposition disposition =
       event_utils::WindowOpenDispositionFromNSEvent([NSApp currentEvent]);
-  browser->OpenURL(node.url, GURL(), disposition,
+  browser->OpenURL(node->url, GURL(), disposition,
                    PageTransition::AUTO_BOOKMARK);
 }
 
-- (HistoryMenuBridge::HistoryItem)itemForTag:(NSInteger)tag {
-  std::vector<HistoryMenuBridge::HistoryItem>* results = NULL;
+- (const HistoryMenuBridge::HistoryItem*)itemForTag:(NSInteger)tag {
+  const ScopedVector<HistoryMenuBridge::HistoryItem>* results = NULL;
   NSInteger tag_base = 0;
   if (tag > IDC_HISTORY_MENU_VISITED && tag < IDC_HISTORY_MENU_CLOSED) {
     results = bridge_->visited_results();
@@ -48,14 +49,14 @@
   DCHECK(tag > tag_base);
   size_t index = tag - tag_base - 1;
   if (index >= results->size())
-    return HistoryMenuBridge::HistoryItem();
+    return NULL;
   return (*results)[index];
 }
 
 - (IBAction)openHistoryMenuItem:(id)sender {
   NSInteger tag = [sender tag];
-  HistoryMenuBridge::HistoryItem item = [self itemForTag:tag];
-  DCHECK(item.url.is_valid());
+  const HistoryMenuBridge::HistoryItem* item = [self itemForTag:tag];
+  DCHECK(item->url.is_valid());
   [self openURLForItem:item];
 }
 
diff --git a/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm b/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm
index 112f798..6bd64f5 100644
--- a/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm
+++ b/chrome/browser/cocoa/history_menu_cocoa_controller_unittest.mm
@@ -1,7 +1,8 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 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 "base/scoped_vector.h"
 #include "base/sys_string_conversions.h"
 #include "chrome/app/chrome_dll_resource.h"
 #include "chrome/browser/browser.h"
@@ -12,6 +13,7 @@
 @interface FakeHistoryMenuController : HistoryMenuCocoaController {
  @public
   BOOL opened_[2];
+  ScopedVector<HistoryMenuBridge::HistoryItem> items_;
 }
 @end
 
@@ -25,22 +27,24 @@
   return self;
 }
 
-- (HistoryMenuBridge::HistoryItem)itemForTag:(NSInteger)tag {
-  HistoryMenuBridge::HistoryItem item;
+- (HistoryMenuBridge::HistoryItem*)itemForTag:(NSInteger)tag {
+  HistoryMenuBridge::HistoryItem* item = new HistoryMenuBridge::HistoryItem();
   if (tag == 0) {
-    item.title = ASCIIToUTF16("uno");
-    item.url = GURL("http://google.com");
+    item->title = ASCIIToUTF16("uno");
+    item->url = GURL("http://google.com");
   } else if (tag == 1) {
-    item.title = ASCIIToUTF16("duo");
-    item.url = GURL("http://apple.com");
+    item->title = ASCIIToUTF16("duo");
+    item->url = GURL("http://apple.com");
   } else {
     NOTREACHED();
   }
+  // We push the item into a scoped vector that will delete it on destruction.
+  items_.push_back(item);
   return item;
 }
 
-- (void)openURLForItem:(HistoryMenuBridge::HistoryItem&)item {
-  std::string url = item.url.possibly_invalid_spec();
+- (void)openURLForItem:(HistoryMenuBridge::HistoryItem*)item {
+  std::string url = item->url.possibly_invalid_spec();
   if (url.find("http://google.com") != std::string::npos)
     opened_[0] = YES;
   if (url.find("http://apple.com") != std::string::npos)
@@ -49,8 +53,8 @@
 
 @end  // FakeHistoryMenuController
 
-TEST(HistoryMenuCocoaControllerTest, TestOpenItem) {
-  FakeHistoryMenuController *c = [[FakeHistoryMenuController alloc] init];
+TEST(HistoryMenuCocoaControllerTest, OpenURLForItem) {
+  FakeHistoryMenuController* c = [[FakeHistoryMenuController alloc] init];
   NSMenuItem* item = [[[NSMenuItem alloc] init] autorelease];
   for (int i = 0; i < 2; ++i) {
     [item setTag:i];