Fix favicon exact match logic and add test.

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 31 matching lines @@
   return history::INVALID_ICON;
 }
 
 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);
 }
 
+bool UrlMatches(const GURL& gurl_a, const GURL& gurl_b) {
+  std::string a = gurl_a.spec();
+  std::string b = gurl_b.spec();
+  // Compare the URL spec without the fragment.
+  return a.substr(0, a.rfind('#')) ==  b.substr(0, b.rfind('#'));

[stevenjb, 2012/03/24 19:07:56]

This addresses a pre-existing inconsistency in favicon behavior that I
discovered while testing. Sites like GMail use fragments, and a complete favicon
update does not appear to occur when just the fragment changes.

[sky, 2012/03/25 03:59:23]

This makes me nervous as I'm not sure of the rules of GURL parsing. Doesn't GURL
have a way to get the fragment directly?

[stevenjb, 2012/03/25 18:38:23]

Good point, trimming gurl.ref() will be much more robust.
Done.

+}
+
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 
 FaviconHandler::DownloadRequest::DownloadRequest()
     : icon_type(history::INVALID_ICON) {
 }
 
 FaviconHandler::DownloadRequest::~DownloadRequest() {
 }
@@ skipping 85 matching lines @@
 
 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 bitmap_size = std::max(bitmap.width(), bitmap.height());
+  bool exact_match = (bitmap_size == preferred_icon_size());
   if (preferred_icon_size() == 0) {
+    // No preferred size, use this icon.
     update_candidate = true;
     exact_match = true;
   } else if (favicon_candidate_.icon_type == history::INVALID_ICON) {
     // No current candidate, use this.
     update_candidate = true;

[stevenjb, 2012/03/24 19:07:56]

The real bug was that exact_match was not getting set correctly here.

   } else {
     if (bitmap_size == preferred_icon_size()) {
       // Exact match, use this.
       update_candidate = true;
-      exact_match = true;
     } else {
       // Compare against current candidate.
       int cur_size = favicon_candidate_.bitmap_size;
       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) {
@@ skipping 13 matching lines @@
       (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) {
+  if (UrlMatches(url, url_) && icon_type == history::FAVICON) {
     NavigationEntry* entry = GetEntry();
     if (entry) {
       entry->GetFavicon().url = image_url;
       UpdateFavicon(entry, &sized_image);
     }
   }
 }
 
 void FaviconHandler::UpdateFavicon(NavigationEntry* entry,
                                    scoped_refptr<RefCountedMemory> data) {
@@ skipping 101 matching lines @@
       // Reset candidate.
       image_urls_.clear();
       favicon_candidate_ = FaviconCandidate();
     }
   }
   download_requests_.erase(i);
 }
 
 NavigationEntry* FaviconHandler::GetEntry() {
   NavigationEntry* entry = delegate_->GetActiveEntry();
-  if (entry && entry->GetURL() == url_)
+  if (entry && UrlMatches(entry->GetURL(), url_))
     return entry;
 
   // If the URL has changed out from under us (as will happen with redirects)
   // return NULL.
   return NULL;
 }
 
 int FaviconHandler::DownloadFavicon(const GURL& image_url, int image_size) {
   if (!image_url.is_valid()) {
     NOTREACHED();
@@ skipping 187 matching lines @@
   int height = bitmap.height();
   if (width > 0 && height > 0) {
     gfx::CalculateFaviconTargetSize(&width, &height);
     return gfx::Image(skia::ImageOperations::Resize(
                           bitmap, skia::ImageOperations::RESIZE_LANCZOS3,
                           width, height));
   }
 
   return image;
 }