Prioritize smaller favicons over larger ones for tabs, etc.

chrome/browser/favicon/favicon_handler.cc

Index: chrome/browser/favicon/favicon_handler.cc
diff --git a/chrome/browser/favicon/favicon_handler.cc b/chrome/browser/favicon/favicon_handler.cc
index 52e429c28a6886e33307cd5758e0e3bb29ea12f3..daa7098243a29cee42544756d45679d7a927ae59 100644
--- a/chrome/browser/favicon/favicon_handler.cc
+++ b/chrome/browser/favicon/favicon_handler.cc
@@ -51,6 +51,8 @@ bool DoUrlAndIconMatch(const FaviconURL& favicon_url,
 
 }  // namespace
 
+////////////////////////////////////////////////////////////////////////////////
+
 FaviconHandler::DownloadRequest::DownloadRequest()
     : icon_type(history::INVALID_ICON) {
 }
@@ -69,6 +71,30 @@ FaviconHandler::DownloadRequest::DownloadRequest(
       icon_type(icon_type) {
 }
 
+////////////////////////////////////////////////////////////////////////////////
+
+FaviconHandler::FaviconCandidate::FaviconCandidate()
+    : icon_type(history::INVALID_ICON) {
+}
+
+FaviconHandler::FaviconCandidate::~FaviconCandidate() {
+}
+
+FaviconHandler::FaviconCandidate::FaviconCandidate(
+    const GURL& url,
+    const GURL& image_url,
+    const gfx::Image& image,
+    const SkBitmap& bitmap,
+    history::IconType icon_type)
+    : url(url),
+      image_url(image_url),
+      image(image),
+      bitmap(bitmap),
+      icon_type(icon_type) {
+}
+
+////////////////////////////////////////////////////////////////////////////////
+
 FaviconHandler::FaviconHandler(Profile* profile,
                                FaviconHandlerDelegate* delegate,
                                Type icon_type)
@@ -76,7 +102,6 @@ FaviconHandler::FaviconHandler(Profile* profile,
       favicon_expired_(false),
       icon_types_(icon_type == FAVICON ? history::FAVICON :
           history::TOUCH_ICON | history::TOUCH_PRECOMPOSED_ICON),
-      current_url_index_(0),
       profile_(profile),
       delegate_(delegate) {
   DCHECK(profile_);
@@ -99,8 +124,6 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   url_ = url;
 
   favicon_expired_ = got_favicon_from_history_ = false;
-  current_url_index_ = 0;
-  urls_.clear();
 
   // Request the favicon from the history service. In parallel to this the
   // renderer is going to notify us (well TabContents) when the favicon url is
@@ -124,12 +147,48 @@ FaviconService* FaviconHandler::GetFaviconService() {
   return profile_->GetFaviconService(Profile::EXPLICIT_ACCESS);
 }
 
+bool FaviconHandler::UpdateFaviconCandidate(const GURL& url,
+                                            const GURL& image_url,
+                                            const gfx::Image& image,
+                                            history::IconType icon_type) {
+  bool update_candidate = false;
+  bool exact_match = false;
+  SkBitmap bitmap = *(image.ToSkBitmap());
+  if (preferred_icon_size() == 0) {
+    update_candidate = true;
+    exact_match = true;
+  } else if (favicon_candidate_.icon_type == history::INVALID_ICON) {
+    // No current candidate, use this.
+    update_candidate = true;
+  } else {
+    int bitmap_size = std::max(bitmap.width(), bitmap.height());
+    if (bitmap_size == preferred_icon_size()) {
+      // Exact match, use this.
+      update_candidate = true;
+      exact_match = true;
+    } else {
+      // Compare against current candidate.
+      int cur_size = std::max(favicon_candidate_.bitmap.width(),
+                              favicon_candidate_.bitmap.height());
+      if ((bitmap_size >= preferred_icon_size() && bitmap_size < cur_size) ||
+          (cur_size < preferred_icon_size() && bitmap_size > cur_size)) {
+        update_candidate = true;
+      }
+    }
+  }
+  if (update_candidate) {
+    favicon_candidate_ = FaviconCandidate(
+        url, image_url, image, bitmap, icon_type);
+  }
+  return exact_match;
+}
+
 void FaviconHandler::SetFavicon(
     const GURL& url,
     const GURL& image_url,
     const gfx::Image& image,
+    const SkBitmap& bitmap,
     history::IconType icon_type) {
-  const SkBitmap& bitmap = image;
   const gfx::Image& sized_image = (preferred_icon_size() == 0 ||
       (preferred_icon_size() == bitmap.width() &&
        preferred_icon_size() == bitmap.height())) ?
@@ -143,8 +202,10 @@ void FaviconHandler::SetFavicon(
 
   if (url == url_ && icon_type == history::FAVICON) {
     NavigationEntry* entry = GetEntry();
-    if (entry)
+    if (entry) {
+      entry->GetFavicon().url = image_url;
       UpdateFavicon(entry, &sized_image);
+    }
   }
 }
 
@@ -170,32 +231,33 @@ void FaviconHandler::UpdateFavicon(NavigationEntry* entry,
 void FaviconHandler::OnUpdateFaviconURL(

[sky, 2012/03/19 20:34:56]

I'm pretty sure that the page can trigger this to be invoked via javascript. Is
that going to be problematic?

[stevenjb, 2012/03/19 20:48:26]

As long as this is always getting called with a complete set of urls, this
should be fine. If we were in the middle of updating (i.e. image_urls_ is not
empty and we are waiting for a callback), we will start over with the new list.
When OnDidDownloadFavicon() gets called with the stale url, the call to
DoUrlAndIconMatch() will fail and that data will be ignored.

Put another way, every call to OnUpdateFaviconURL() will generate a new complete
list of urls to consider, and the closest match from that list will be used. 

     int32 page_id,
     const std::vector<FaviconURL>& candidates) {
-  NavigationEntry* entry = GetEntry();
-  if (!entry)
-    return;
 
-  bool got_favicon_url_update = false;
+  image_urls_.clear();
+  favicon_candidate_ = FaviconCandidate();
   for (std::vector<FaviconURL>::const_iterator i = candidates.begin();
        i != candidates.end(); ++i) {
-    if (!i->icon_url.is_empty() && (i->icon_type & icon_types_)) {
-      if (!got_favicon_url_update) {
-        got_favicon_url_update = true;
-        urls_.clear();
-        current_url_index_ = 0;
-      }
-      urls_.push_back(*i);
-    }
+    if (!i->icon_url.is_empty() && (i->icon_type & icon_types_))
+      image_urls_.push_back(*i);
   }
 
   // TODO(davemoore) Should clear on empty url. Currently we ignore it.
   // This appears to be what FF does as well.
-  // No URL was added.
-  if (!got_favicon_url_update)
+  if (image_urls_.empty())
     return;
 
   if (!GetFaviconService())
     return;
 
+  ProcessCurrentUrl();
+}
+
+void FaviconHandler::ProcessCurrentUrl() {
+  DCHECK(!image_urls_.empty());
+
+  NavigationEntry* entry = GetEntry();
+  if (!entry)
+    return;
+
   // For FAVICON.
   if (current_candidate()->icon_type == FaviconURL::FAVICON) {
     if (!favicon_expired_ && entry->GetFavicon().valid &&
@@ -235,13 +297,22 @@ void FaviconHandler::OnDidDownloadFavicon(int id,
                                i->second.icon_type)) {
     // The downloaded icon is still valid when there is no FaviconURL update
     // during the downloading.
+    bool request_next_icon = true;
     if (!errored) {
-      SetFavicon(i->second.url, image_url, image, i->second.icon_type);
-    } else if (GetEntry() && ++current_url_index_ < urls_.size()) {
-      // Copies all candidate except first one and notifies the FaviconHandler,
-      // so the next candidate can be processed.
-      std::vector<FaviconURL> new_candidates(urls_.begin() + 1, urls_.end());
-      OnUpdateFaviconURL(0, new_candidates);
+      request_next_icon = !UpdateFaviconCandidate(
+          i->second.url, image_url, image, i->second.icon_type);
+    }
+    if (request_next_icon && GetEntry() && image_urls_.size() > 1) {
+      // Remove the first member of image_urls_ and process the remaining.
+      image_urls_.pop_front();
+      ProcessCurrentUrl();
+    } else if (favicon_candidate_.icon_type != history::INVALID_ICON) {
+      // No more icons to request, set the favicon from the candidate.
+      SetFavicon(favicon_candidate_.url, favicon_candidate_.image_url,
+                 favicon_candidate_.image, favicon_candidate_.bitmap,
+                 favicon_candidate_.icon_type);
+      // Reset candidate.
+      favicon_candidate_ = FaviconCandidate();
     }
   }
   download_requests_.erase(i);