Retrieve favicons from history as NavigationEntries are converted from TabNav...

chrome/browser/ui/toolbar/back_forward_menu_model.cc

Index: chrome/browser/ui/toolbar/back_forward_menu_model.cc
diff --git a/chrome/browser/ui/toolbar/back_forward_menu_model.cc b/chrome/browser/ui/toolbar/back_forward_menu_model.cc
index 67943d9e93d4ed3cba91856a77728a90822106ee..eb1da27c775cb6cf313f8f127691e3eeba243e0b 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),
+      delegate_(NULL) {
 }
 
 bool BackForwardMenuModel::HasIcons() const {
@@ -110,7 +112,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;
 
@@ -120,6 +122,9 @@ bool BackForwardMenuModel::GetIconAt(int index, SkBitmap* icon) const {
   } else {
     NavigationEntry* entry = GetNavigationEntry(index);
     *icon = entry->favicon().bitmap();
+    if (!entry->favicon().is_valid()) {
+      FetchFavicon(entry, index);
+    }
   }
 
   return true;
@@ -179,6 +184,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 {
@@ -200,6 +207,67 @@ bool BackForwardMenuModel::IsSeparator(int index) const {
   return index == history_items;
 }
 
+void BackForwardMenuModel::SetDelegate(Delegate* delegate) {
+  delegate_ = delegate;
+}
+
+void BackForwardMenuModel::FetchFavicon(NavigationEntry* entry, int index) {

[sky, 2011/02/22 18:28:28]

It doesn't look like you use index here, can it be nuked?

[dill, 2011/02/24 15:00:28]

Yes, that was a leftover from when unique_id wasn't used.

On 2011/02/22 18:28:28, sky wrote:

+  // 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(), &load_consumer_,
+      NewCallback(this, &BackForwardMenuModel::OnFavIconDataAvailable));
+  load_consumer_.SetClientData(favicon_service, handle, entry->unique_id());
+}
+
+void BackForwardMenuModel::OnFavIconDataAvailable(
+    FaviconService::Handle handle,
+    bool know_favicon,
+    scoped_refptr<RefCountedMemory> data,
+    bool expired,
+    GURL icon_url) {
+  if (know_favicon && data.get() && data->size() && data->size() > 0) {

[sky, 2011/02/22 18:28:28]

How come there is data->size() && data->size() > 0 ?
Isn't one enough?
Also, make this an early return if false.

[dill, 2011/02/24 15:00:28]

data->size() > 0 was from an earlier code review, didn't make sense to me but I
added it. I should have pushed back. Early return is already there, this if
statement covers the rest of the function.

On 2011/02/22 18:28:28, sky wrote:

+    int unique_id = load_consumer_.GetClientDataForCurrentRequest();
+    // We need both the NavigationEntry and the model_index so we search for

[sky, 2011/02/22 18:28:28]

This comment isn't too helpful. The useful comment to add is between 253 and 254
explaining why entry may be null.

[dill, 2011/02/24 15:00:28]

How about here I say "Find the current model_index for the unique_id." and on
line 253 I say "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."

On 2011/02/22 18:28:28, sky wrote:

[sky, 2011/02/24 15:12:50]

Sounds good.

+    // the unique_index by model_index.
+    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)
+      return;
+    // Now that we have a valid NavigationEntry, decode the favicon and assign
+    // it to the NavigationEntry.
+    SkBitmap fav_icon;
+    if (gfx::PNGCodec::Decode(data->front(), data->size(), &fav_icon)) {
+      entry->favicon().set_is_valid(true);
+      entry->favicon().set_url(icon_url);
+      if (fav_icon.empty())
+        return;
+      entry->favicon().set_bitmap(fav_icon);
+      if (delegate_) {
+        delegate_->OnIconChanged(model_index);
+      }
+    }
+  }
+}
+
 int BackForwardMenuModel::GetHistoryItemCount() const {
   TabContents* contents = GetTabContents();
   int items = 0;