Sync the bookmark's icon URL.

BUG=160726
Test=TwoClientBookmarksSyncTest.*

Review URL: https://chromiumcodereview.appspot.com/11428004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170326 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 164 insertions, 59 deletions

diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index e55e552..9e98e5f 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -144,6 +144,7 @@ void BookmarkNode::Initialize(int64 id) {
 }
 
 void BookmarkNode::InvalidateFavicon() {
+  icon_url_ = GURL();
   favicon_ = gfx::Image();
   favicon_state_ = INVALID_FAVICON;
 }
@@ -894,6 +895,7 @@ void BookmarkModel::OnFaviconDataAvailable(
   node->set_favicon_state(BookmarkNode::LOADED_FAVICON);
   if (!image_result.image.IsEmpty()) {
     node->set_favicon(image_result.image);
+    node->set_icon_url(image_result.icon_url);
     FaviconLoaded(node);
   }
 }
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index b5089e7..f4c263bf2 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -79,6 +79,10 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
   const GURL& url() const { return url_; }
   void set_url(const GURL& url) { url_ = url; }
 
+  // Returns the favicon's URL. Returns an empty URL if there is no favicon
+  // associated with this bookmark.
+  const GURL& icon_url() const { return icon_url_; }
+
   Type type() const { return type_; }
   void set_type(Type type) { type_ = type; }
 
@@ -133,6 +137,11 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
   // Called when the favicon becomes invalid.
   void InvalidateFavicon();
 
+  // Sets the favicon's URL.
+  void set_icon_url(const GURL& icon_url) {
+    icon_url_ = icon_url;
+  }
+
   const gfx::Image& favicon() const { return favicon_; }
   void set_favicon(const gfx::Image& icon) { favicon_ = icon; }
 
@@ -165,6 +174,9 @@ class BookmarkNode : public ui::TreeNode<BookmarkNode> {
   // The favicon of this node.
   gfx::Image favicon_;
 
+  // The URL of the node's favicon.
+  GURL icon_url_;
+
   // The loading state of the favicon.
   FaviconState favicon_state_;
 
diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc
index d7f0e15..ee17b7e 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,
     const 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());
     sync_node->SetBookmarkSpecifics(updated_specifics);
   }
 }
diff --git a/chrome/browser/sync/glue/bookmark_change_processor.h b/chrome/browser/sync/glue/bookmark_change_processor.h
index 749e121..58e03be 100644
--- a/chrome/browser/sync/glue/bookmark_change_processor.h
+++ b/chrome/browser/sync/glue/bookmark_change_processor.h
@@ -93,12 +93,13 @@ class BookmarkChangeProcessor : public BookmarkModelObserver,
                                  const BookmarkNode* bookmark_node,
                                  BookmarkModel* model);
 
-  // Applies the favicon 1x |bitmap_data| to |bookmark_node|.
+  // Applies the 1x favicon |bitmap_data| and |icon_url| to |bookmark_node|.
   // |profile| is the profile that contains the HistoryService and BookmarkModel
   // for the bookmark in question.
   static void ApplyBookmarkFavicon(
       const BookmarkNode* bookmark_node,
       Profile* profile,
+      const GURL& icon_url,
       const scoped_refptr<base::RefCountedMemory>& bitmap_data);
 
   // Sets the favicon of the given sync node from the given bookmark node.
diff --git a/chrome/browser/sync/test/integration/bookmarks_helper.cc b/chrome/browser/sync/test/integration/bookmarks_helper.cc
index 856797d..608f107 100644
--- a/chrome/browser/sync/test/integration/bookmarks_helper.cc
+++ b/chrome/browser/sync/test/integration/bookmarks_helper.cc
@@ -15,6 +15,7 @@
 #include "chrome/browser/bookmarks/bookmark_model_observer.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
+#include "chrome/browser/favicon/favicon_util.h"
 #include "chrome/browser/history/history_service_factory.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile.h"
@@ -25,7 +26,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/base/models/tree_node_iterator.h"
-#include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/image/image_skia.h"
 
 using sync_datatype_helper::test;
 
@@ -163,13 +164,32 @@ bool FaviconBitmapsMatch(const SkBitmap& bitmap_a, const SkBitmap& bitmap_b) {
   }
 }
 
-// Gets the favicon associated with |node| in |model|.
-gfx::Image GetFavicon(BookmarkModel* model, const BookmarkNode* node) {
+// Represents a favicon image and the icon URL associated with it.
+struct FaviconData {
+  FaviconData() {
+  }
+
+  FaviconData(const gfx::Image& favicon_image,
+              const GURL& favicon_url)
+      : image(favicon_image),
+        icon_url(favicon_url) {
+  }
+
+  ~FaviconData() {
+  }
+
+  gfx::Image image;
+  GURL icon_url;
+};
+
+// Gets the favicon and icon URL associated with |node| in |model|.
+FaviconData GetFaviconData(BookmarkModel* model,
+                           const BookmarkNode* node) {
   // If a favicon wasn't explicitly set for a particular URL, simply return its
   // blank favicon.
   if (!urls_with_favicons_ ||
       urls_with_favicons_->find(node->url()) == urls_with_favicons_->end()) {
-    return gfx::Image();
+    return FaviconData();
   }
   // If a favicon was explicitly set, we may need to wait for it to be loaded
   // via BookmarkModel::GetFavicon(), which is an asynchronous operation.
@@ -180,13 +200,14 @@ gfx::Image GetFavicon(BookmarkModel* model, const BookmarkNode* node) {
   }
   EXPECT_TRUE(node->is_favicon_loaded());
   EXPECT_FALSE(model->GetFavicon(node).IsEmpty());
-  return model->GetFavicon(node);
+  return FaviconData(model->GetFavicon(node), node->icon_url());
 }
 
 // Sets the favicon for |profile| and |node|. |profile| may be
 // |test()->verifier()|.
 void SetFaviconImpl(Profile* profile,
                     const BookmarkNode* node,
+                    const GURL& icon_url,
                     const gfx::Image& image) {
     BookmarkModel* model = BookmarkModelFactory::GetForProfile(profile);
 
@@ -195,7 +216,7 @@ void SetFaviconImpl(Profile* profile,
         FaviconServiceFactory::GetForProfile(profile,
                                              Profile::EXPLICIT_ACCESS);
     favicon_service->SetFavicons(node->url(),
-                                 node->url(),
+                                 icon_url,
                                  history::FAVICON,
                                  image);
 
@@ -203,7 +224,7 @@ void SetFaviconImpl(Profile* profile,
     observer.WaitForSetFavicon();
     // Wait for the BookmarkModel to fetch the updated favicon and for the new
     // favicon to be sent to BookmarkChangeProcessor.
-    GetFavicon(model, node);
+    GetFaviconData(model, node);
 }
 
 // Wait for all currently scheduled tasks on the history thread for all
@@ -241,13 +262,27 @@ bool FaviconsMatch(BookmarkModel* model_a,
                    BookmarkModel* model_b,
                    const BookmarkNode* node_a,
                    const BookmarkNode* node_b) {
-  const gfx::Image& bitmap_a = GetFavicon(model_a, node_a);
-  const gfx::Image& bitmap_b = GetFavicon(model_b, node_b);
+  FaviconData favicon_data_a = GetFaviconData(model_a, node_a);
+  FaviconData favicon_data_b = GetFaviconData(model_b, node_b);
+
+  if (favicon_data_a.icon_url != favicon_data_b.icon_url)
+    return false;
+
+  gfx::Image image_a = favicon_data_a.image;
+  gfx::Image image_b = favicon_data_b.image;
 
-  if (bitmap_a.IsEmpty() && bitmap_b.IsEmpty())
+  if (image_a.IsEmpty() && image_b.IsEmpty())
     return true;  // Two empty images are equivalent.
-  return !bitmap_a.IsEmpty() && !bitmap_b.IsEmpty() &&
-          FaviconBitmapsMatch(*bitmap_a.ToSkBitmap(), *bitmap_b.ToSkBitmap());
+
+  if (image_a.IsEmpty() != image_b.IsEmpty())
+    return false;
+
+  // Compare only the 1x bitmaps as only those are synced.
+  SkBitmap bitmap_a = image_a.AsImageSkia().GetRepresentation(
+      ui::SCALE_FACTOR_100P).sk_bitmap();
+  SkBitmap bitmap_b = image_b.AsImageSkia().GetRepresentation(
+      ui::SCALE_FACTOR_100P).sk_bitmap();
+  return FaviconBitmapsMatch(bitmap_a, bitmap_b);
 }
 
 // Does a deep comparison of BookmarkNode fields in |model_a| and |model_b|.
@@ -457,10 +492,8 @@ void SetTitle(int profile,
 
 void SetFavicon(int profile,
                 const BookmarkNode* node,
-                const std::vector<unsigned char>& icon_bytes_vector) {
-  scoped_refptr<base::RefCountedBytes> bitmap_data(
-      new base::RefCountedBytes(icon_bytes_vector));
-  gfx::Image image(bitmap_data->front(), bitmap_data->size());
+                const GURL& icon_url,
+                const gfx::Image& image) {
   ASSERT_EQ(GetBookmarkModel(profile)->GetNodeByID(node->id()), node)
       << "Node " << node->GetTitle() << " does not belong to "
       << "Profile " << profile;
@@ -472,9 +505,9 @@ void SetFavicon(int profile,
   if (test()->use_verifier()) {
     const BookmarkNode* v_node = NULL;
     FindNodeInVerifier(GetBookmarkModel(profile), node, &v_node);
-    SetFaviconImpl(test()->verifier(), v_node, image);
+    SetFaviconImpl(test()->verifier(), v_node, icon_url, image);
   }
-  SetFaviconImpl(test()->GetProfile(profile), node, image);
+  SetFaviconImpl(test()->GetProfile(profile), node, icon_url, image);
 }
 
 const BookmarkNode* SetURL(int profile,
@@ -653,22 +686,23 @@ int CountFoldersWithTitlesMatching(int profile, const std::wstring& title) {
                                       WideToUTF16(title));
 }
 
-std::vector<unsigned char> CreateFavicon(int seed) {
-  const int w = 16;
-  const int h = 16;
-  SkBitmap bmp;
-  bmp.setConfig(SkBitmap::kARGB_8888_Config, w, h);
-  bmp.allocPixels();
-  uint32_t* src_data = bmp.getAddr32(0, 0);
-  for (int i = 0; i < w * h; ++i) {
-    src_data[i] = SkPreMultiplyARGB((seed + i) % 255,
-                                    (seed + i) % 250,
-                                    (seed + i) % 245,
-                                    (seed + i) % 240);
-  }
-  std::vector<unsigned char> favicon;
-  gfx::PNGCodec::EncodeBGRASkBitmap(bmp, false, &favicon);
-  return favicon;
+gfx::Image CreateFavicon(SkColor color) {
+  const int dip_width = 16;
+  const int dip_height = 16;
+  std::vector<ui::ScaleFactor> favicon_scale_factors =
+      FaviconUtil::GetFaviconScaleFactors();
+  gfx::ImageSkia favicon;
+  for (size_t i = 0; i < favicon_scale_factors.size(); ++i) {
+    float scale = ui::GetScaleFactorScale(favicon_scale_factors[i]);
+    int pixel_width = dip_width * scale;
+    int pixel_height = dip_height * scale;
+    SkBitmap bmp;
+    bmp.setConfig(SkBitmap::kARGB_8888_Config, pixel_width, pixel_height);
+    bmp.allocPixels();
+    bmp.eraseColor(color);
+    favicon.AddRepresentation(gfx::ImageSkiaRep(bmp, favicon_scale_factors[i]));
+  }
+  return gfx::Image(favicon);
 }
 
 std::string IndexedURL(int i) {
diff --git a/chrome/browser/sync/test/integration/bookmarks_helper.h b/chrome/browser/sync/test/integration/bookmarks_helper.h
index d1373ca..f69310b 100644
--- a/chrome/browser/sync/test/integration/bookmarks_helper.h
+++ b/chrome/browser/sync/test/integration/bookmarks_helper.h
@@ -88,12 +88,12 @@ void SetTitle(int profile,
                      const BookmarkNode* node,
                      const std::wstring& new_title);
 
-// Sets the favicon of the node |node| (of type BookmarkNode::URL) in the
-// bookmark model of profile |profile| using the data in |icon_bytes_vector|.
-void SetFavicon(
-    int profile,
-    const BookmarkNode* node,
-    const std::vector<unsigned char>& icon_bytes_vector);
+// Sets the |icon_url| and |image| data for the favicon for |node| in the
+// bookmark model for |profile|.
+void SetFavicon(int profile,
+                const BookmarkNode* node,
+                const GURL& icon_url,
+                const gfx::Image& image);
 
 // Changes the url of the node |node| in the bookmark model of profile
 // |profile| to |new_url|. Returns a pointer to the node with the changed url.
@@ -165,8 +165,9 @@ int CountFoldersWithTitlesMatching(
     int profile,
     const std::wstring& title) WARN_UNUSED_RESULT;
 
-// Creates a unique favicon using |seed|.
-std::vector<unsigned char> CreateFavicon(int seed);
+// Creates a favicon of |color| with image reps of the platform's supported
+// scale factors (eg MacOS) in addition to 1x.
+gfx::Image CreateFavicon(SkColor color);
 
 // Returns a URL identifiable by |i|.
 std::string IndexedURL(int i);
diff --git a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc b/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
index 1f3c197..ca45a01 100644
--- a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
@@ -53,10 +53,6 @@ class TwoClientBookmarksSyncTest : public SyncTest {
   DISALLOW_COPY_AND_ASSIGN(TwoClientBookmarksSyncTest);
 };
 
-const std::vector<unsigned char> GenericFavicon() {
-  return CreateFavicon(254);
-}
-
 IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, Sanity) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllModelsMatchVerifier());
@@ -142,10 +138,52 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_AddFirstBMWithFavicon) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
   ASSERT_TRUE(AllModelsMatchVerifier());
 
-  const BookmarkNode* bookmark = AddURL(0, kGenericURLTitle, GURL(kGenericURL));
+  const GURL page_url(kGenericURL);
+  const GURL icon_url("http://www.google.com/favicon.ico");
+
+  const BookmarkNode* bookmark = AddURL(0, kGenericURLTitle, page_url);
+
   ASSERT_TRUE(bookmark != NULL);
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
-  SetFavicon(0, bookmark, GenericFavicon());
+  SetFavicon(0, bookmark, icon_url, CreateFavicon(SK_ColorWHITE));
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  ASSERT_TRUE(AllModelsMatchVerifier());
+}
+
+// Test that the history service logic for not losing the hidpi versions of
+// favicons as a result of sync does not result in dropping sync updates.
+// In particular, the synced 16x16 favicon bitmap should overwrite 16x16
+// favicon bitmaps on all clients. (Though non-16x16 favicon bitmaps
+// are unchanged).
+IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_SetFaviconHiDPI) {
+  // Set the supported scale factors to include 2x such that CreateFavicon()
+  // creates a favicon with hidpi representations and that methods in the
+  // FaviconService request hidpi favicons.
+  std::vector<ui::ScaleFactor> supported_scale_factors;
+  supported_scale_factors.push_back(ui::SCALE_FACTOR_100P);
+  supported_scale_factors.push_back(ui::SCALE_FACTOR_200P);
+  ui::test::SetSupportedScaleFactors(supported_scale_factors);
+
+  const GURL page_url(kGenericURL);
+  const GURL icon_url1("http://www.google.com/favicon1.ico");
+  const GURL icon_url2("http://www.google.com/favicon2.ico");
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  const BookmarkNode* bookmark0 = AddURL(0, kGenericURLTitle, page_url);
+  ASSERT_TRUE(bookmark0 != NULL);
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  SetFavicon(0, bookmark0, icon_url1, CreateFavicon(SK_ColorWHITE));
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  const BookmarkNode* bookmark1 = GetUniqueNodeByURL(1, page_url);
+  SetFavicon(1, bookmark1, icon_url1, CreateFavicon(SK_ColorBLUE));
+  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  SetFavicon(0, bookmark0, icon_url2, CreateFavicon(SK_ColorGREEN));
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
   ASSERT_TRUE(AllModelsMatchVerifier());
 }
diff --git a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
index 314633f..2c4ef16 100644
--- a/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_typed_urls_sync_test.cc
@@ -443,11 +443,13 @@ IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest,
 
 IN_PROC_BROWSER_TEST_F(TwoClientTypedUrlsSyncTest, BookmarksWithTypedVisit) {
   GURL bookmark_url("http://www.bookmark.google.com/");
+  GURL bookmark_icon_url("http://www.bookmark.google.com/favicon.ico");
   ASSERT_TRUE(SetupClients());
   // Create a bookmark.
   const BookmarkNode* node = bookmarks_helper::AddURL(
       0, bookmarks_helper::IndexedURLTitle(0), bookmark_url);
-  bookmarks_helper::SetFavicon(0, node, bookmarks_helper::CreateFavicon(254));
+  bookmarks_helper::SetFavicon(0, node, bookmark_icon_url,
+      bookmarks_helper::CreateFavicon(SK_ColorWHITE));
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
 
   ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
diff --git a/sync/protocol/bookmark_specifics.proto b/sync/protocol/bookmark_specifics.proto
index 31721ab..efb03e5 100644
--- a/sync/protocol/bookmark_specifics.proto
+++ b/sync/protocol/bookmark_specifics.proto
@@ -22,5 +22,6 @@ message BookmarkSpecifics {
   // Corresponds to BookmarkNode::date_added() and is the internal value from
   // base::Time.
   optional int64 creation_time_us = 4;
+  optional string icon_url = 5;
 }
 
diff --git a/sync/protocol/proto_value_conversions.cc b/sync/protocol/proto_value_conversions.cc
index df151f7..f6b9322 100644
--- a/sync/protocol/proto_value_conversions.cc
+++ b/sync/protocol/proto_value_conversions.cc
@@ -290,6 +290,7 @@ DictionaryValue* BookmarkSpecificsToValue(
   SET_BYTES(favicon);
   SET_STR(title);
   SET_INT64(creation_time_us);
+  SET_STR(icon_url);
   return value;
 }
 
diff --git a/sync/protocol/proto_value_conversions_unittest.cc b/sync/protocol/proto_value_conversions_unittest.cc
index e87888c..642037c 100644
--- a/sync/protocol/proto_value_conversions_unittest.cc
+++ b/sync/protocol/proto_value_conversions_unittest.cc
@@ -124,13 +124,18 @@ TEST_F(ProtoValueConversionsTest, BookmarkSpecificsToValue) {
 
 TEST_F(ProtoValueConversionsTest, BookmarkSpecificsData) {
   const base::Time creation_time(base::Time::Now());
+  const std::string icon_url = "http://www.google.com/favicon.ico";
   sync_pb::BookmarkSpecifics specifics;
   specifics.set_creation_time_us(creation_time.ToInternalValue());
+  specifics.set_icon_url(icon_url);
   scoped_ptr<DictionaryValue> value(BookmarkSpecificsToValue(specifics));
   EXPECT_FALSE(value->empty());
   std::string encoded_time;
   EXPECT_TRUE(value->GetString("creation_time_us", &encoded_time));
   EXPECT_EQ(base::Int64ToString(creation_time.ToInternalValue()), encoded_time);
+  std::string encoded_icon_url;
+  EXPECT_TRUE(value->GetString("icon_url", &encoded_icon_url));
+  EXPECT_EQ(icon_url, encoded_icon_url);
 }
 
 TEST_F(ProtoValueConversionsTest, DeviceInfoSpecificsToValue) {