Fix favicon exact match logic and add test.

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 a94a8d7e8d2908810964f43a6cfac987c9b0b207..de4a031f5e280545aed9fa0c6c7a222cea609dfd 100644
--- a/chrome/browser/favicon/favicon_handler.cc
+++ b/chrome/browser/favicon/favicon_handler.cc
@@ -49,6 +49,13 @@ bool DoUrlAndIconMatch(const FaviconURL& favicon_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
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -154,10 +161,11 @@ bool FaviconHandler::UpdateFaviconCandidate(const GURL& 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) {
@@ -167,7 +175,6 @@ bool FaviconHandler::UpdateFaviconCandidate(const GURL& url,
     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;
@@ -201,7 +208,7 @@ void FaviconHandler::SetFavicon(
       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;
@@ -323,7 +330,7 @@ void FaviconHandler::OnDidDownloadFavicon(int id,
 
 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)