[Sync:USS] Don't always load favicon when creating bookmark specifics

Before this CL: favicon is always loaded when creating specifics out of
a bookmark node.

After this CL: a new paramter is added to control this behavior and
ignores the favicon field if the favicon is not loaded already.

This is added such that in debug page chrome://sync-internals,
the load of a favicon is avoid if not loaded already.

Bug: 516866
Change-Id: I14093cea131d44e83cca1b5ab07d64d6b46061c1
Reviewed-on: https://chromium-review.googlesource.com/c/1254066
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596550}

components/sync_bookmarks/bookmark_local_changes_builder.cc

@@ -66,7 +66,8 @@ BookmarkLocalChangesBuilder::BuildCommitRequests(size_t max_entries) const {
       data.unique_position = metadata->unique_position();
       // Assign specifics only for the non-deletion case. In case of deletion,
       // EntityData should contain empty specifics to indicate deletion.
-      data.specifics = CreateSpecificsFromBookmarkNode(node, bookmark_model_);
+      data.specifics = CreateSpecificsFromBookmarkNode(
+          node, bookmark_model_, /*force_favicon_load=*/true);
     }
     request.entity = data.PassToPtr();
     request.sequence_number = metadata->sequence_number();

components/sync_bookmarks/bookmark_model_merger.cc

@@ -334,8 +334,8 @@ void BookmarkModelMerger::ProcessLocalCreation(
   }
 
   const bookmarks::BookmarkNode* node = parent->GetChild(index);
-  const sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, bookmark_model_);
+  const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, bookmark_model_, /*force_favicon_load=*/true);
   bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
                          pos.ToProto(), specifics);
   // Mark the entity that it needs to be committed.

components/sync_bookmarks/bookmark_model_observer_impl.cc

@@ -63,7 +63,7 @@ void BookmarkModelObserverImpl::BookmarkNodeMoved(
       ComputePosition(*new_parent, new_index, sync_id).ToProto();
 
   sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model);
+      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
 
   bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
                             modification_time, unique_position, specifics);
@@ -98,7 +98,7 @@ void BookmarkModelObserverImpl::BookmarkNodeAdded(
       ComputePosition(*parent, index, sync_id).ToProto();
 
   sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model);
+      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
 
   bookmark_tracker_->Add(sync_id, node, server_version, creation_time,
                          unique_position, specifics);
@@ -180,7 +180,7 @@ void BookmarkModelObserverImpl::BookmarkNodeChanged(
   const std::string& sync_id = entity->metadata()->server_id();
   const base::Time modification_time = base::Time::Now();
   sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model);
+      CreateSpecificsFromBookmarkNode(node, model, /*force_favicon_load=*/true);
   if (entity->MatchesSpecificsHash(specifics)) {
     // We should push data to the server only if there is an actual change in
     // the data. We could hit this code path without having actual changes
@@ -262,8 +262,8 @@ void BookmarkModelObserverImpl::BookmarkNodeChildrenReordered(
 
     previous_position = position;
 
-    const sync_pb::EntitySpecifics specifics =
-        CreateSpecificsFromBookmarkNode(node, model);
+    const sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+        node, model, /*force_favicon_load=*/true);
 
     bookmark_tracker_->Update(sync_id, entity->metadata()->server_version(),
                               modification_time, position.ToProto(), specifics);

components/sync_bookmarks/bookmark_model_type_processor.cc

@@ -451,7 +451,8 @@ void BookmarkModelTypeProcessor::AppendNodeAndChildrenForDebugging(
   data.non_unique_name = base::UTF16ToUTF8(node->GetTitle());
   data.is_folder = node->is_folder();
   data.unique_position = metadata->unique_position();
-  data.specifics = CreateSpecificsFromBookmarkNode(node, bookmark_model_);
+  data.specifics = CreateSpecificsFromBookmarkNode(
+      node, bookmark_model_, /*force_favicon_load=*/false);
   if (node->is_permanent_node()) {
     data.server_defined_unique_tag =
         ComputeServerDefinedUniqueTagForDebugging(node, bookmark_model_);

components/sync_bookmarks/bookmark_specifics_conversions.cc

@@ -89,7 +89,8 @@ void SetBookmarkFaviconFromSpecifics(
 
 sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
     const bookmarks::BookmarkNode* node,
-    bookmarks::BookmarkModel* model) {
+    bookmarks::BookmarkModel* model,
+    bool force_favicon_load) {
   sync_pb::EntitySpecifics specifics;
   sync_pb::BookmarkSpecifics* bm_specifics = specifics.mutable_bookmark();
   if (!node->is_folder()) {
@@ -99,6 +100,14 @@ sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
   bm_specifics->set_creation_time_us(
       node->date_added().ToDeltaSinceWindowsEpoch().InMicroseconds());
 
+  if (node->GetMetaInfoMap()) {
+    UpdateBookmarkSpecificsMetaInfo(node->GetMetaInfoMap(), bm_specifics);
+  }
+
+  if (!force_favicon_load && !node->is_favicon_loaded()) {
+    return specifics;
+  }
+
   // Encodes a bookmark's favicon into raw PNG data.
   scoped_refptr<base::RefCountedMemory> favicon_bytes(nullptr);
   const gfx::Image& favicon = model->GetFavicon(node);
@@ -122,9 +131,6 @@ sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
     bm_specifics->clear_icon_url();
   }
 
-  if (node->GetMetaInfoMap()) {
-    UpdateBookmarkSpecificsMetaInfo(node->GetMetaInfoMap(), bm_specifics);
-  }
   return specifics;
 }
 

components/sync_bookmarks/bookmark_specifics_conversions.h

@@ -23,7 +23,8 @@ namespace sync_bookmarks {
 
 sync_pb::EntitySpecifics CreateSpecificsFromBookmarkNode(
     const bookmarks::BookmarkNode* node,
-    bookmarks::BookmarkModel* model);
+    bookmarks::BookmarkModel* model,
+    bool force_favicon_load);
 
 // Creates a bookmark node under the given parent node from the given specifics.
 // Returns the newly created node. Callers must verify that

components/sync_bookmarks/bookmark_specifics_conversions_unittest.cc

@@ -6,7 +6,9 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
+#include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "components/bookmarks/browser/bookmark_model.h"
@@ -26,6 +28,29 @@ namespace sync_bookmarks {
 
 namespace {
 
+class TestBookmarkClientWithFaviconLoad : public bookmarks::TestBookmarkClient {
+ public:
+  TestBookmarkClientWithFaviconLoad() = default;
+  ~TestBookmarkClientWithFaviconLoad() override = default;
+
+  base::CancelableTaskTracker::TaskId GetFaviconImageForPageURL(
+      const GURL& page_url,
+      favicon_base::IconType type,
+      const favicon_base::FaviconImageCallback& callback,
+      base::CancelableTaskTracker* tracker) override {
+    ++load_favicon_requests;
+    return TestBookmarkClient::GetFaviconImageForPageURL(page_url, type,
+                                                         callback, tracker);
+  }
+
+  int GetLoadFaviconRequestsForTest() { return load_favicon_requests; }
+
+ private:
+  int load_favicon_requests = 0;
+
+  DISALLOW_COPY_AND_ASSIGN(TestBookmarkClientWithFaviconLoad);
+};
+
 TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) {
   const GURL kUrl("http://www.url.com");
   const std::string kTitle = "Title";
@@ -41,14 +66,14 @@ TEST(BookmarkSpecificsConversionsTest, ShouldCreateSpecificsFromBookmarkNode) {
   const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
   const bookmarks::BookmarkNode* node = model->AddURL(
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16(kTitle),
-      GURL(kUrl));
+      kUrl);
   ASSERT_THAT(node, NotNull());
   model->SetDateAdded(node, kTime);
   model->SetNodeMetaInfo(node, kKey1, kValue1);
   model->SetNodeMetaInfo(node, kKey2, kValue2);
 
-  sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model.get());
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model.get(), /*force_favicon_load=*/false);
   const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark();
   EXPECT_THAT(bm_specifics.title(), Eq(kTitle));
   EXPECT_THAT(GURL(bm_specifics.url()), Eq(kUrl));
@@ -72,12 +97,54 @@ TEST(BookmarkSpecificsConversionsTest,
       /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("Title"));
   ASSERT_THAT(node, NotNull());
 
-  sync_pb::EntitySpecifics specifics =
-      CreateSpecificsFromBookmarkNode(node, model.get());
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model.get(), /*force_favicon_load=*/false);
   const sync_pb::BookmarkSpecifics& bm_specifics = specifics.bookmark();
   EXPECT_FALSE(bm_specifics.has_url());
 }
 
+TEST(BookmarkSpecificsConversionsTest,
+     ShouldLoadFaviconWhenCreatingSpecificsFromBookmarkNode) {
+  auto client = std::make_unique<TestBookmarkClientWithFaviconLoad>();
+  TestBookmarkClientWithFaviconLoad* client_ptr = client.get();
+
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      TestBookmarkClientWithFaviconLoad::CreateModelWithClient(
+          std::move(client));
+
+  const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* node = model->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("Title"),
+      GURL("http://www.url.com"));
+  ASSERT_THAT(node, NotNull());
+  ASSERT_FALSE(node->is_favicon_loaded());
+  ASSERT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(0));
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model.get(), /*force_favicon_load=*/true);
+  EXPECT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(1));
+}
+
+TEST(BookmarkSpecificsConversionsTest,
+     ShouldNotLoadFaviconWhenCreatingSpecificsFromBookmarkNode) {
+  auto client = std::make_unique<TestBookmarkClientWithFaviconLoad>();
+  TestBookmarkClientWithFaviconLoad* client_ptr = client.get();
+
+  std::unique_ptr<bookmarks::BookmarkModel> model =
+      TestBookmarkClientWithFaviconLoad::CreateModelWithClient(
+          std::move(client));
+
+  const bookmarks::BookmarkNode* bookmark_bar_node = model->bookmark_bar_node();
+  const bookmarks::BookmarkNode* node = model->AddURL(
+      /*parent=*/bookmark_bar_node, /*index=*/0, base::UTF8ToUTF16("Title"),
+      GURL("http://www.url.com"));
+  ASSERT_THAT(node, NotNull());
+  ASSERT_FALSE(node->is_favicon_loaded());
+  ASSERT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(0));
+  sync_pb::EntitySpecifics specifics = CreateSpecificsFromBookmarkNode(
+      node, model.get(), /*force_favicon_load=*/false);
+  EXPECT_THAT(client_ptr->GetLoadFaviconRequestsForTest(), Eq(0));
+}
+
 TEST(BookmarkSpecificsConversionsTest, ShouldCreateBookmarkNodeFromSpecifics) {
   const GURL kUrl("http://www.url.com");
   const std::string kTitle = "Title";