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

chrome/browser/ui/toolbar/back_forward_menu_model.cc

 // 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 "build/build_config.h"
 
 #include "chrome/browser/ui/toolbar/back_forward_menu_model.h"
 
 #include "base/string_number_conversions.h"
 #include "chrome/browser/metrics/user_metrics.h"
 #include "chrome/browser/tab_contents/navigation_controller.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "net/base/registry_controlled_domain.h"
 #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;
 static const int kMaxWidth = 700;
 
 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 {
   return true;
 }
 
 int BackForwardMenuModel::GetItemCount() const {
   int items = GetHistoryItemCount();
 
   if (items > 0) {
@@ skipping 58 matching lines @@
 }
 
 bool BackForwardMenuModel::IsItemCheckedAt(int index) const {
   return false;
 }
 
 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;
 
   if (index == GetItemCount() - 1) {
     *icon = *ResourceBundle::GetSharedInstance().GetBitmapNamed(
         IDR_HISTORY_FAVICON);
   } else {
     NavigationEntry* entry = GetNavigationEntry(index);
     *icon = entry->favicon().bitmap();
+    if (!entry->favicon().is_valid()) {
+      FetchFavicon(entry, index);
+    }
   }
 
   return true;
 }
 
 ui::ButtonMenuItemModel* BackForwardMenuModel::GetButtonMenuItemAt(
     int index) const {
   return NULL;
 }
 
@@ skipping 39 matching lines @@
   int controller_index = MenuIndexToNavEntryIndex(index);
   if (!browser_->NavigateToIndexWithDisposition(
           controller_index, static_cast<WindowOpenDisposition>(disposition))) {
     NOTREACHED();
   }
 }
 
 void BackForwardMenuModel::MenuWillShow() {
   UserMetrics::RecordComputedAction(BuildActionName("Popup", -1),
                                     browser_->profile());
+  requested_favicons_.clear();
+  load_consumer_.CancelAllRequests();
 }
 
 bool BackForwardMenuModel::IsSeparator(int index) const {
   int history_items = GetHistoryItemCount();
   // If the index is past the number of history items + separator,
   // we then consider if it is a chapter-stop entry.
   if (index > history_items) {
     // We either are in ChapterStop area, or at the end of the list (the "Show
     // Full History" link).
     int chapter_stops = GetChapterStopCount(history_items);
     if (chapter_stops == 0)
       return false;  // We must have reached the "Show Full History" link.
     // Otherwise, look to see if we have reached the separator for the
     // chapter-stops. If not, this is a chapter stop.
     return (index == history_items + 1 + chapter_stops);
   }
 
   // Look to see if we have reached the separator for the history items.
   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;
 
   if (model_type_ == FORWARD_MENU) {
     // Only count items from n+1 to end (if n is current entry)
     items = contents->controller().entry_count() -
             contents->controller().GetCurrentEntryIndex() - 1;
   } else {
     items = contents->controller().GetCurrentEntryIndex();
@@ skipping 161 matching lines @@
     metric_string += "ForwardMenu_";
   else
     metric_string += "BackMenu_";
   metric_string += action;
   if (index != -1) {
     // +1 is for historical reasons (indices used to start at 1).
     metric_string += base::IntToString(index + 1);
   }
   return metric_string;
 }