Prioritize smaller favicons over larger ones for tabs, etc.

chrome/browser/favicon/favicon_handler.cc

 // Copyright (c) 2012 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 "chrome/browser/favicon/favicon_handler.h"
 
 #include "build/build_config.h"
 
 #include <vector>
 
@@ skipping 33 matching lines @@
 
 bool DoUrlAndIconMatch(const FaviconURL& favicon_url,
                        const GURL& url,
                        history::IconType icon_type) {
   return favicon_url.icon_url == url &&
       favicon_url.icon_type == static_cast<FaviconURL::IconType>(icon_type);
 }
 
 }  // namespace
 
+////////////////////////////////////////////////////////////////////////////////
+
 FaviconHandler::DownloadRequest::DownloadRequest()
     : icon_type(history::INVALID_ICON) {
 }
 
 FaviconHandler::DownloadRequest::~DownloadRequest() {
 }
 
 FaviconHandler::DownloadRequest::DownloadRequest(
     const GURL& url,
     const GURL& image_url,
     const FaviconTabHelper::ImageDownloadCallback& callback,
     history::IconType icon_type)
     : url(url),
       image_url(image_url),
       callback(callback),
       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)
     : got_favicon_from_history_(false),
       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) {
@@ skipping 12 matching lines @@
 }
 
 void FaviconHandler::FetchFavicon(const GURL& url) {
   cancelable_consumer_.CancelAllRequests();
 
   url_ = 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
   // available.
   if (GetFaviconService()) {
     GetFaviconForURL(url_, icon_types_, &cancelable_consumer_,
         base::Bind(&FaviconHandler::OnFaviconDataForInitialURL,
                    base::Unretained(this)));
   }
 }
 
 int FaviconHandler::DownloadImage(
     const GURL& image_url,
     int image_size,
     history::IconType icon_type,
     const FaviconTabHelper::ImageDownloadCallback& callback) {
   return ScheduleDownload(GURL(), image_url, image_size, icon_type, callback);
 }
 
 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())) ?
       image : ResizeFaviconIfNeeded(image);
 
   if (GetFaviconService() && ShouldSaveFavicon(url)) {
     std::vector<unsigned char> image_data;
     if (gfx::PNGEncodedDataFromImage(sized_image, &image_data))
       SetHistoryFavicon(url, image_url, image_data, icon_type);
   }
 
   if (url == url_ && icon_type == history::FAVICON) {
     NavigationEntry* entry = GetEntry();
-    if (entry)
+    if (entry) {
+      entry->GetFavicon().url = image_url;
       UpdateFavicon(entry, &sized_image);
+    }
   }
 }
 
 void FaviconHandler::UpdateFavicon(NavigationEntry* entry,
                                    scoped_refptr<RefCountedMemory> data) {
   scoped_ptr<gfx::Image> image(gfx::ImageFromPNGEncodedData(data->front(),
                                                             data->size()));
   UpdateFavicon(entry, image.get());
 }
 
@@ skipping 70 matching lines @@
     return;
   }
 
   if (!i->second.callback.is_null()) {
     i->second.callback.Run(id, errored, *(&image));
   } else if (current_candidate() &&
              DoUrlAndIconMatch(*current_candidate(), image_url,
                                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);
 }
 
 NavigationEntry* FaviconHandler::GetEntry() {
   NavigationEntry* entry = delegate_->GetActiveEntry();
   if (entry && entry->GetURL() == url_)
     return entry;
 
@@ skipping 196 matching lines @@
   if (width > 0 && height > 0) {
     gfx::CalculateFaviconTargetSize(&width, &height);
     return gfx::Image(new SkBitmap(
                           skia::ImageOperations::Resize(
                               bitmap, skia::ImageOperations::RESIZE_LANCZOS3,
                               width, height)));
   }
 
   return image;
 }