Sync the bookmark's icon URL

chrome/browser/sync/glue/bookmark_change_processor.cc

Index: chrome/browser/sync/glue/bookmark_change_processor.cc
diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc
index 17c5e31986161c45901e32130abfbf569f85465e..79cc3015f6b0e1260e6a81ee1e0d610d98467041 100644
--- a/chrome/browser/sync/glue/bookmark_change_processor.cc
+++ b/chrome/browser/sync/glue/bookmark_change_processor.cc
@@ -700,15 +700,22 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon(
     BookmarkModel* bookmark_model) {
   const sync_pb::BookmarkSpecifics& specifics =
       sync_node->GetBookmarkSpecifics();
-  const std::string& icon_str = specifics.favicon();
-  if (icon_str.empty())
+  const std::string& icon_bytes_str = specifics.favicon();
+  if (icon_bytes_str.empty())
     return false;
 
   scoped_refptr<base::RefCountedString> icon_bytes(
       new base::RefCountedString());
-  icon_bytes->data().assign(icon_str);
+  icon_bytes->data().assign(icon_bytes_str);
+  GURL icon_url(specifics.icon_url());
 
-  ApplyBookmarkFavicon(bookmark_node, bookmark_model->profile(),
+  // Old clients may not be syncing the favicon URL. If the icon URL is not
+  // synced, use the page URL as a fake icon URL as it is guaranteed to be
+  // unique.
+  if (icon_url.is_empty())
+    icon_url = bookmark_node->url();
+
+  ApplyBookmarkFavicon(bookmark_node, bookmark_model->profile(), icon_url,
                        icon_bytes);
 
   return true;
@@ -718,6 +725,7 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon(
 void BookmarkChangeProcessor::ApplyBookmarkFavicon(
     const BookmarkNode* bookmark_node,
     Profile* profile,
+    const GURL& icon_url,
     scoped_refptr<base::RefCountedMemory> bitmap_data) {
   HistoryService* history =
       HistoryServiceFactory::GetForProfile(profile, Profile::EXPLICIT_ACCESS);
@@ -727,13 +735,12 @@ void BookmarkChangeProcessor::ApplyBookmarkFavicon(
   history->AddPageNoVisitForBookmark(bookmark_node->url(),
                                      bookmark_node->GetTitle());
   // The client may have cached the favicon at 2x. Use MergeFavicon() as not to
-  // overwrite the cached 2x favicon bitmap. Use the page URL as a fake icon URL
-  // as it is guaranteed to be unique. Sync favicons are always
-  // gfx::kFaviconSize in width and height. Store the favicon into history as
-  // such.
+  // overwrite the cached 2x favicon bitmap. Sync favicons are always
+  // gfx::kFaviconSize in width and height. Store the favicon into history
+  // as such.
   gfx::Size pixel_size(gfx::kFaviconSize, gfx::kFaviconSize);
   favicon_service->MergeFavicon(bookmark_node->url(),
-                                bookmark_node->url(),
+                                icon_url,
                                 history::FAVICON,
                                 bitmap_data,
                                 pixel_size);
@@ -750,6 +757,7 @@ void BookmarkChangeProcessor::SetSyncNodeFavicon(
     sync_pb::BookmarkSpecifics updated_specifics(
         sync_node->GetBookmarkSpecifics());
     updated_specifics.set_favicon(&favicon_bytes[0], favicon_bytes.size());
+    updated_specifics.set_icon_url(bookmark_node->icon_url().spec());

[Nicolas Zea, 2012/11/27 18:56:09]

I'm a bit worried about this. Is there a possibility that we'll wind up
overwriting a valid favicon url with a legacy favicon url (i.e. one that's the
same as the bookmark url?)

[pkotwicz, 2012/11/27 22:50:39]

I'm not sure I completely understand your concern.
The bookmark node's icon URL reflects the icon URL as per the history database.
I know for a fact that by design we overwrite whatever icon URL the history
service has with the one gotten from sync when MergeFavicon() is called.
So, if a new bookmark is created on a legacy client, the bookmark will be
created on clients that the legacy client is synced with with a fake icon URL.
I do not know how we would get into a situation where we wind up overwriting a
valid icon url with a legacy icon url.

     sync_node->SetBookmarkSpecifics(updated_specifics);
   }
 }