Lands http://codereview.chromium.org/2928005/ for Dill:

Loads favicons when openning back/forward menu for any urls that don't
have favicons.

BUG=5679
TEST=Restore a tab with a navigation history, check favicons.

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

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

9 files changed, 220 insertions, 14 deletions

diff --git a/chrome/browser/chromeos/status/clock_menu_button.h b/chrome/browser/chromeos/status/clock_menu_button.h
index 3f85a2b..434309d 100644
--- a/chrome/browser/chromeos/status/clock_menu_button.h
+++ b/chrome/browser/chromeos/status/clock_menu_button.h
@@ -42,7 +42,7 @@ class ClockMenuButton : public StatusAreaButton,
       ui::Accelerator* accelerator) const { return false; }
   virtual bool IsItemCheckedAt(int index) const { return false; }
   virtual int GetGroupIdAt(int index) const { return 0; }
-  virtual bool GetIconAt(int index, SkBitmap* icon) const { return false; }
+  virtual bool GetIconAt(int index, SkBitmap* icon) { return false; }
   virtual ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const {
     return NULL;
   }
@@ -51,6 +51,7 @@ class ClockMenuButton : public StatusAreaButton,
   virtual void HighlightChangedTo(int index) {}
   virtual void ActivatedAt(int index);
   virtual void MenuWillShow() {}
+  virtual void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) {}
 
   // Overridden from ResumeLibrary::Observer:
   virtual void PowerChanged(PowerLibrary* obj) {}
diff --git a/chrome/browser/chromeos/status/input_method_menu.cc b/chrome/browser/chromeos/status/input_method_menu.cc
index e004cf6..305871a 100644
--- a/chrome/browser/chromeos/status/input_method_menu.cc
+++ b/chrome/browser/chromeos/status/input_method_menu.cc
@@ -221,7 +221,7 @@ bool InputMethodMenu::HasIcons() const  {
   return false;
 }
 
-bool InputMethodMenu::GetIconAt(int index, SkBitmap* icon) const {
+bool InputMethodMenu::GetIconAt(int index, SkBitmap* icon) {
   return false;
 }
 
@@ -249,6 +249,10 @@ void InputMethodMenu::MenuWillShow() {
   // Views for Chromium OS does not support this interface yet.
 }
 
+void InputMethodMenu::SetMenuModelDelegate(ui::MenuModelDelegate* delegate) {
+  // Not needed for current usage.
+}
+
 int InputMethodMenu::GetItemCount() const {
   if (!model_.get()) {
     // Model is not constructed yet. This means that
diff --git a/chrome/browser/chromeos/status/input_method_menu.h b/chrome/browser/chromeos/status/input_method_menu.h
index a336395..e377d61 100644
--- a/chrome/browser/chromeos/status/input_method_menu.h
+++ b/chrome/browser/chromeos/status/input_method_menu.h
@@ -48,13 +48,14 @@ class InputMethodMenu : public views::ViewMenuDelegate,
                                 ui::Accelerator* accelerator) const;
   virtual bool IsItemCheckedAt(int index) const;
   virtual int GetGroupIdAt(int index) const;
-  virtual bool GetIconAt(int index, SkBitmap* icon) const;
+  virtual bool GetIconAt(int index, SkBitmap* icon);
   virtual ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const;
   virtual bool IsEnabledAt(int index) const;
   virtual ui::MenuModel* GetSubmenuModelAt(int index) const;
   virtual void HighlightChangedTo(int index);
   virtual void ActivatedAt(int index);
   virtual void MenuWillShow();
+  virtual void SetMenuModelDelegate(ui::MenuModelDelegate* delegate);
 
   // views::ViewMenuDelegate implementation. Sub classes can override the method
   // to adjust the position of the menu.
diff --git a/chrome/browser/chromeos/status/network_menu.cc b/chrome/browser/chromeos/status/network_menu.cc
index 2415490..f68454e 100644
--- a/chrome/browser/chromeos/status/network_menu.cc
+++ b/chrome/browser/chromeos/status/network_menu.cc
@@ -279,7 +279,7 @@ bool NetworkMenu::IsItemCheckedAt(int index) const {
   return true;
 }
 
-bool NetworkMenu::GetIconAt(int index, SkBitmap* icon) const {
+bool NetworkMenu::GetIconAt(int index, SkBitmap* icon) {
   if (!menu_items_[index].icon.empty()) {
     *icon = menu_items_[index].icon;
     return true;
diff --git a/chrome/browser/chromeos/status/network_menu.h b/chrome/browser/chromeos/status/network_menu.h
index 9187799..f96d49f 100644
--- a/chrome/browser/chromeos/status/network_menu.h
+++ b/chrome/browser/chromeos/status/network_menu.h
@@ -99,7 +99,7 @@ class NetworkMenu : public views::ViewMenuDelegate,
       ui::Accelerator* accelerator) const { return false; }
   virtual bool IsItemCheckedAt(int index) const;
   virtual int GetGroupIdAt(int index) const { return 0; }
-  virtual bool GetIconAt(int index, SkBitmap* icon) const;
+  virtual bool GetIconAt(int index, SkBitmap* icon);
   virtual ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const {
     return NULL;
   }
@@ -108,6 +108,7 @@ class NetworkMenu : public views::ViewMenuDelegate,
   virtual void HighlightChangedTo(int index) {}
   virtual void ActivatedAt(int index);
   virtual void MenuWillShow() {}
+  virtual void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) {}
 
   void SetFirstLevelMenuWidth(int width);
 
diff --git a/chrome/browser/chromeos/status/power_menu_button.h b/chrome/browser/chromeos/status/power_menu_button.h
index a839e63..709d2bc4 100644
--- a/chrome/browser/chromeos/status/power_menu_button.h
+++ b/chrome/browser/chromeos/status/power_menu_button.h
@@ -41,7 +41,7 @@ class PowerMenuButton : public StatusAreaButton,
       ui::Accelerator* accelerator) const { return false; }
   virtual bool IsItemCheckedAt(int index) const { return false; }
   virtual int GetGroupIdAt(int index) const { return 0; }
-  virtual bool GetIconAt(int index, SkBitmap* icon) const { return false; }
+  virtual bool GetIconAt(int index, SkBitmap* icon) { return false; }
   virtual ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const {
     return NULL;
   }
@@ -50,6 +50,7 @@ class PowerMenuButton : public StatusAreaButton,
   virtual void HighlightChangedTo(int index) {}
   virtual void ActivatedAt(int index) {}
   virtual void MenuWillShow() {}
+  virtual void SetMenuModelDelegate(ui::MenuModelDelegate* delegate) {}
 
   // PowerLibrary::Observer implementation.
   virtual void PowerChanged(PowerLibrary* obj);
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model.cc b/chrome/browser/ui/toolbar/back_forward_menu_model.cc
index c77c894..cd74736 100644
--- a/chrome/browser/ui/toolbar/back_forward_menu_model.cc
+++ b/chrome/browser/ui/toolbar/back_forward_menu_model.cc
@@ -22,6 +22,7 @@
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/text/text_elider.h"
+#include "ui/gfx/codec/png_codec.h"
 
 const int BackForwardMenuModel::kMaxHistoryItems = 12;
 const int BackForwardMenuModel::kMaxChapterStops = 5;
@@ -31,7 +32,8 @@ BackForwardMenuModel::BackForwardMenuModel(Browser* browser,
                                            ModelType model_type)
     : browser_(browser),
       test_tab_contents_(NULL),
-      model_type_(model_type) {
+      model_type_(model_type),
+      menu_model_delegate_(NULL) {
 }
 
 bool BackForwardMenuModel::HasIcons() const {
@@ -113,7 +115,7 @@ int BackForwardMenuModel::GetGroupIdAt(int index) const {
   return false;
 }
 
-bool BackForwardMenuModel::GetIconAt(int index, SkBitmap* icon) const {
+bool BackForwardMenuModel::GetIconAt(int index, SkBitmap* icon) {
   if (!ItemHasIcon(index))
     return false;
 
@@ -123,6 +125,9 @@ bool BackForwardMenuModel::GetIconAt(int index, SkBitmap* icon) const {
   } else {
     NavigationEntry* entry = GetNavigationEntry(index);
     *icon = entry->favicon().bitmap();
+    if (!entry->favicon().is_valid() && menu_model_delegate()) {
+      FetchFavicon(entry);
+    }
   }
 
   return true;
@@ -182,6 +187,8 @@ void BackForwardMenuModel::ActivatedAtWithDisposition(
 void BackForwardMenuModel::MenuWillShow() {
   UserMetrics::RecordComputedAction(BuildActionName("Popup", -1),
                                     browser_->profile());
+  requested_favicons_.clear();
+  load_consumer_.CancelAllRequests();
 }
 
 bool BackForwardMenuModel::IsSeparator(int index) const {
@@ -203,6 +210,71 @@ bool BackForwardMenuModel::IsSeparator(int index) const {
   return index == history_items;
 }
 
+void BackForwardMenuModel::SetMenuModelDelegate(
+      ui::MenuModelDelegate* menu_model_delegate) {
+  menu_model_delegate_ = menu_model_delegate;
+}
+
+void BackForwardMenuModel::FetchFavicon(NavigationEntry* entry) {
+  // If the favicon has already been requested for this menu, don't do
+  // anything.
+  if (requested_favicons_.find(entry->unique_id()) !=
+      requested_favicons_.end()) {
+    return;
+  }
+  requested_favicons_.insert(entry->unique_id());
+  FaviconService* favicon_service =
+      browser_->profile()->GetFaviconService(Profile::EXPLICIT_ACCESS);
+  if (!favicon_service)
+    return;
+  FaviconService::Handle handle = favicon_service->GetFaviconForURL(
+      entry->url(), history::FAVICON, &load_consumer_,
+      NewCallback(this, &BackForwardMenuModel::OnFavIconDataAvailable));
+  load_consumer_.SetClientData(favicon_service, handle, entry->unique_id());
+}
+
+void BackForwardMenuModel::OnFavIconDataAvailable(
+    FaviconService::Handle handle,
+    history::FaviconData favicon) {
+  if (favicon.is_valid()) {
+    int unique_id = load_consumer_.GetClientDataForCurrentRequest();
+    // Find the current model_index for the unique_id.
+    NavigationEntry* entry = NULL;
+    int model_index = -1;
+    for (int i = 0; i < GetItemCount() - 1; i++) {
+      if (IsSeparator(i))
+        continue;
+      if (GetNavigationEntry(i)->unique_id() == unique_id) {
+        model_index = i;
+        entry = GetNavigationEntry(i);
+        break;
+      }
+    }
+
+    if (!entry)
+      // The NavigationEntry wasn't found, this can happen if the user
+      // navigates to another page and a NavigatationEntry falls out of the
+      // range of kMaxHistoryItems.
+      return;
+
+    // Now that we have a valid NavigationEntry, decode the favicon and assign
+    // it to the NavigationEntry.
+    SkBitmap fav_icon;
+    if (gfx::PNGCodec::Decode(favicon.image_data->front(),
+                              favicon.image_data->size(),
+                              &fav_icon)) {
+      entry->favicon().set_is_valid(true);
+      entry->favicon().set_url(favicon.icon_url);
+      if (fav_icon.empty())
+        return;
+      entry->favicon().set_bitmap(fav_icon);
+      if (menu_model_delegate()) {
+        menu_model_delegate()->OnIconChanged(model_index);
+      }
+    }
+  }
+}
+
 int BackForwardMenuModel::GetHistoryItemCount() const {
   TabContents* contents = GetTabContents();
   int items = 0;
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model.h b/chrome/browser/ui/toolbar/back_forward_menu_model.h
index b1bb08c..655369d 100644
--- a/chrome/browser/ui/toolbar/back_forward_menu_model.h
+++ b/chrome/browser/ui/toolbar/back_forward_menu_model.h
@@ -6,11 +6,13 @@
 #define CHROME_BROWSER_UI_TOOLBAR_BACK_FORWARD_MENU_MODEL_H_
 #pragma once
 
+#include <set>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/string16.h"
+#include "chrome/browser/favicon_service.h"
 #include "ui/base/models/menu_model.h"
 #include "webkit/glue/window_open_disposition.h"
 
@@ -53,7 +55,7 @@ class BackForwardMenuModel : public ui::MenuModel {
                                 ui::Accelerator* accelerator) const;
   virtual bool IsItemCheckedAt(int index) const;
   virtual int GetGroupIdAt(int index) const;
-  virtual bool GetIconAt(int index, SkBitmap* icon) const;
+  virtual bool GetIconAt(int index, SkBitmap* icon);
   virtual ui::ButtonMenuItemModel* GetButtonMenuItemAt(int index) const;
   virtual bool IsEnabledAt(int index) const;
   virtual MenuModel* GetSubmenuModelAt(int index) const;
@@ -65,7 +67,28 @@ class BackForwardMenuModel : public ui::MenuModel {
   // Is the item at |index| a separator?
   bool IsSeparator(int index) const;
 
+  // Set the delegate for triggering OnIconChanged.
+  virtual void SetMenuModelDelegate(ui::MenuModelDelegate* menu_model_delegate);
+
+ protected:
+   ui::MenuModelDelegate* menu_model_delegate() { return menu_model_delegate_; }
+
  private:
+  friend class BackFwdMenuModelTest;
+  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, BasicCase);
+  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, MaxItemsTest);
+  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, ChapterStops);
+  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, EscapeLabel);
+  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, FaviconLoadTest);
+
+  // Requests a favicon from the FaviconService. Called by GetIconAt if the
+  // NavigationEntry has an invalid favicon.
+  void FetchFavicon(NavigationEntry* entry);
+
+  // Callback from the favicon service.
+  void OnFavIconDataAvailable(FaviconService::Handle handle,
+                              history::FaviconData favicon);
+
   // Allows the unit test to use its own dummy tab contents.
   void set_test_tab_contents(TabContents* test_tab_contents) {
     test_tab_contents_ = test_tab_contents;
@@ -160,11 +183,15 @@ class BackForwardMenuModel : public ui::MenuModel {
   // back button.
   ModelType model_type_;
 
-  friend class BackFwdMenuModelTest;
-  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, BasicCase);
-  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, MaxItemsTest);
-  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, ChapterStops);
-  FRIEND_TEST_ALL_PREFIXES(BackFwdMenuModelTest, EscapeLabel);
+  // Keeps track of which favicons have already been requested from the history
+  // to prevent duplicate requests, identified by NavigationEntry->unique_id().
+  std::set<int> requested_favicons_;
+
+  // Used for loading favicons from history.
+  CancelableRequestConsumerTSimple<int> load_consumer_;
+
+  // Used for receiving notifications when an icon is changed.
+  ui::MenuModelDelegate* menu_model_delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(BackForwardMenuModel);
 };
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc b/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
index 5e8d1d4..93a284c 100644
--- a/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/back_forward_menu_model_unittest.cc
@@ -7,14 +7,50 @@
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/history/history.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/test/testing_profile.h"
+#include "content/browser/browser_thread.h"
 #include "content/browser/renderer_host/test_render_view_host.h"
 #include "content/browser/tab_contents/navigation_controller.h"
 #include "content/browser/tab_contents/navigation_entry.h"
 #include "content/browser/tab_contents/tab_contents.h"
 #include "content/browser/tab_contents/test_tab_contents.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+#include "ui/gfx/codec/png_codec.h"
+
+namespace {
+
+// Creates a bitmap of the specified color.
+SkBitmap CreateBitmap(SkColor color) {
+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config, 16, 16);
+  bitmap.allocPixels();
+  bitmap.eraseColor(color);
+  return bitmap;
+}
+
+class FaviconDelegate : public ui::MenuModelDelegate {
+ public:
+  FaviconDelegate() : was_called_(false) {}
+
+  void OnIconChanged(int model_index) {
+    was_called_ = true;
+    MessageLoop::current()->Quit();
+  }
+
+  bool was_called() const { return was_called_; }
+
+ private:
+  bool was_called_;
+
+  DISALLOW_COPY_AND_ASSIGN(FaviconDelegate);
+};
+
+}  // namespace
 
 class BackFwdMenuModelTest : public RenderViewHostTestHarness {
  public:
@@ -450,3 +486,66 @@ TEST_F(BackFwdMenuModelTest, EscapeLabel) {
   EXPECT_EQ(ASCIIToUTF16("A &&&&&& B"), back_model->GetLabelAt(0));
 #endif // defined(OS_MACOSX)
 }
+
+// Test asynchronous loading of favicon from history service.
+TEST_F(BackFwdMenuModelTest, FaviconLoadTest) {
+  profile()->CreateHistoryService(true, false);
+  profile()->CreateFaviconService();
+  Browser browser(Browser::TYPE_NORMAL, profile());
+  FaviconDelegate favicon_delegate;
+
+  BackForwardMenuModel back_model(
+      &browser, BackForwardMenuModel::BACKWARD_MENU);
+  back_model.set_test_tab_contents(controller().tab_contents());
+  back_model.SetMenuModelDelegate(&favicon_delegate);
+
+  SkBitmap new_icon(CreateBitmap(SK_ColorRED));
+  std::vector<unsigned char> icon_data;
+  gfx::PNGCodec::EncodeBGRASkBitmap(new_icon, false, &icon_data);
+
+  GURL url1 = GURL("http://www.a.com/1");
+  GURL url2 = GURL("http://www.a.com/2");
+  GURL url1_favicon("http://www.a.com/1/favicon.ico");
+
+  NavigateAndCommit(url1);
+  // Navigate to a new URL so that url1 will be in the BackForwardMenuModel.
+  NavigateAndCommit(url2);
+
+  // Set the desired favicon for url1.
+  profile()->GetHistoryService(Profile::EXPLICIT_ACCESS)->AddPage(url1,
+      history::SOURCE_BROWSED);
+  profile()->GetFaviconService(Profile::EXPLICIT_ACCESS)->SetFavicon(url1,
+      url1_favicon, icon_data, history::FAVICON);
+
+  // Will return the current icon (default) but start an anync call
+  // to retrieve the favicon from the favicon service.
+  SkBitmap default_icon;
+  back_model.GetIconAt(0, &default_icon);
+
+  // Make the favicon service run GetFavIconForURL,
+  // FaviconDelegate.OnIconChanged will be called.
+  MessageLoop::current()->Run();
+
+  // Verify that the callback executed.
+  EXPECT_TRUE(favicon_delegate.was_called());
+
+  // Verify the bitmaps match.
+  SkBitmap valid_icon;
+  // This time we will get the new favicon returned.
+  back_model.GetIconAt(0, &valid_icon);
+  SkAutoLockPixels a(new_icon);
+  SkAutoLockPixels b(valid_icon);
+  SkAutoLockPixels c(default_icon);
+  // Verify we did not get the default favicon.
+  EXPECT_NE(0, memcmp(default_icon.getPixels(), valid_icon.getPixels(),
+               default_icon.getSize()));
+  // Verify we did get the expected favicon.
+  EXPECT_EQ(0, memcmp(new_icon.getPixels(), valid_icon.getPixels(),
+              new_icon.getSize()));
+
+  // Make sure the browser deconstructor doesn't have problems.
+  browser.CloseAllTabs();
+  // This is required to prevent the message loop from hanging.
+  profile()->DestroyHistoryService();
+}
+