Sync the bookmark's icon URL

chrome/browser/sync/glue/bookmark_change_processor.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/sync/glue/bookmark_change_processor.h"
 
 #include <stack>
 #include <vector>
 
 #include "base/location.h"
@@ skipping 682 matching lines @@
 }
 
 // static
 // Sets the favicon of the given bookmark node from the given sync node.
 bool BookmarkChangeProcessor::SetBookmarkFavicon(
     syncer::BaseNode* sync_node,
     const BookmarkNode* bookmark_node,
     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;
 }
 
 // static
 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);
   FaviconService* favicon_service =
       FaviconServiceFactory::GetForProfile(profile, Profile::EXPLICIT_ACCESS);
 
   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);
 }
 
 // static
 void BookmarkChangeProcessor::SetSyncNodeFavicon(
     const BookmarkNode* bookmark_node,
     BookmarkModel* model,
     syncer::WriteNode* sync_node) {
   std::vector<unsigned char> favicon_bytes;
   EncodeFavicon(bookmark_node, model, &favicon_bytes);
   if (!favicon_bytes.empty()) {
     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);
   }
 }
 
 }  // namespace browser_sync