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 cc19d92541f23d57081a84c2bf2027bfbd7343d1..8a379f5dabd18d850b0339c1079b38bf791f760f 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)
@@ -101,6 +127,7 @@ void FaviconHandler::FetchFavicon(const GURL& url) {
   favicon_expired_ = got_favicon_from_history_ = false;
   current_url_index_ = 0;
   urls_.clear();
+  favicon_candidate_.icon_type = history::INVALID_ICON;

[michaelbai, 2012/03/15 22:39:53]

It seemed that you should store the best matched candidate with DownloadRequest.
I am afraid that there is race condition here. Imaging that the 2nd response of
DownloadRequest is in the UI queue, and the icon_type is reset to INVALID_ICON
here.

[stevenjb, 2012/03/15 23:29:06]

I believe that the call to CancelAllRequests() should prevent any further calls
to OnDidDownloadFavicon() until a fresh call to OnUpdateFaviconURL().

I can also clear download_requests_ which will prevent processing any current
pending requests.

[sky, 2012/03/16 22:15:09]

CancelAllRequests only cancels requests to the faviconservice, not to the
renderer. I don't know that there is a way to cancel such requests.
If we do that, it means when we might not store an image we downloaded when we
should.

[stevenjb, 2012/03/16 22:31:29]

My understanding is that FetchFavicon() will always be followed by a fresh call
to OnUpdateFaviconURL(). Otherwise the existing code would have the same problem
if the first entry was invalid.

So, by clearing download_requests_, we ignore any pending results and just
process the new list, which is what we want.

I can clean this up some more by splitting OnUpdateFaviconURL() into two parts
if that would help clarify this.

 
   // 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 +151,49 @@ 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());
+  int preferred_size = preferred_icon_size();
+  if (preferred_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_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_size && bitmap_size < cur_size) ||
+          (cur_size < preferred_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 +207,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);
+    }
   }
 }
 
@@ -235,13 +301,26 @@ 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,
+      bool exact_match = UpdateFaviconCandidate(

[michaelbai, 2012/03/15 22:39:53]

nit: You don't need exact_match here.

[stevenjb, 2012/03/15 23:29:06]

I thought the code would be more readable this way, since the return value isn't
entirely obvious from the function name.

+          i->second.url, image_url, image, i->second.icon_type);
+      if (exact_match)
+        request_next_icon = false;
+    }

[michaelbai, 2012/03/15 22:39:53]

You changed the logical here, previously, we didn't try the reset of icons if
error happened. I am not sure if it is a issue, please confirm with sky.

[stevenjb, 2012/03/15 23:29:06]

Previously we tried the remaining icons only if there was an error. 
Now we always try the remaining icons unless there was not an error and the icon
is an exact match.

+    if (request_next_icon &&
+        (GetEntry() && ++current_url_index_ < urls_.size())) {
+      // Copies all candidates except the first 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);
+    } 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_.icon_type = history::INVALID_ICON;

[michaelbai, 2012/03/15 22:39:53]

Could you reset all by favicon_candidate = FaviconCandidate();

[stevenjb, 2012/03/15 23:29:06]

Done.

     }
   }
   download_requests_.erase(i);