[Sync::USS] Implement remote creation of bookmarks

This CL implements the remote creation of bookmarks on the server.
It take many shortcuts that will be addressed in later patches such as
a naive ordering algorithm that assumes all bookmarks are created
under the bookmarks bar directly.

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

chrome/browser/sync/test/integration/single_client_bookmarks_sync_test.cc

@@ -6,6 +6,7 @@
 
 #include "base/macros.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sync/test/integration/bookmarks_helper.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
@@ -14,9 +15,11 @@
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/browser/url_and_title.h"
 #include "components/browser_sync/profile_sync_service.h"
+#include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/test/fake_server/bookmark_entity_builder.h"
 #include "components/sync/test/fake_server/entity_builder_factory.h"
 #include "components/sync/test/fake_server/fake_server_verifier.h"
+#include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/layout.h"
 
 using bookmarks::BookmarkModel;
@@ -45,6 +48,22 @@ using bookmarks_helper::SetTitle;
 // SyncTest and using it in all single client tests.
 const int kSingleProfileIndex = 0;
 
+// Class that enables or disables USS based on test parameter. Must be the first
+// base class of the test fixture.
+class UssSwitchToggler : public testing::WithParamInterface<bool> {
+ public:
+  UssSwitchToggler() {
+    if (GetParam()) {
+      override_features_.InitAndEnableFeature(switches::kSyncUSSBookmarks);
+    } else {
+      override_features_.InitAndDisableFeature(switches::kSyncUSSBookmarks);
+    }
+  }
+
+ private:
+  base::test::ScopedFeatureList override_features_;
+};
+
 class SingleClientBookmarksSyncTest : public SyncTest {
  public:
   SingleClientBookmarksSyncTest() : SyncTest(SINGLE_CLIENT) {}
@@ -76,6 +95,19 @@ void SingleClientBookmarksSyncTest::VerifyBookmarkModelMatchesFakeServer(
   }
 }
 
+// TODO(crbug.com/516866): Merge the two fixtures into one when all tests are
+// passing for USS.
+class SingleClientBookmarksSyncTestIncludingUssTests
+    : public UssSwitchToggler,
+      public SingleClientBookmarksSyncTest {
+ public:
+  SingleClientBookmarksSyncTestIncludingUssTests(){};
+  ~SingleClientBookmarksSyncTestIncludingUssTests() override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SingleClientBookmarksSyncTestIncludingUssTests);
+};
+
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, Sanity) {
   ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
 
@@ -360,7 +392,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
   ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
 }
 
-IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, DownloadBookmark) {
+IN_PROC_BROWSER_TEST_P(SingleClientBookmarksSyncTestIncludingUssTests,
+                       DownloadBookmark) {
   std::string title = "Patrick Star";
   fake_server::EntityBuilderFactory entity_builder_factory;
   fake_server::BookmarkEntityBuilder bookmark_builder =
@@ -463,3 +496,7 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, DownloadBookmarkFolder) {
 IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, E2E_ONLY(SanitySetup)) {
   ASSERT_TRUE(SetupSync()) <<  "SetupSync() failed.";
 }
+
+INSTANTIATE_TEST_CASE_P(USS,
+                        SingleClientBookmarksSyncTestIncludingUssTests,
+                        ::testing::Values(false, true));

components/sync/protocol/bookmark_specifics.proto

@@ -24,8 +24,8 @@ message BookmarkSpecifics {
   optional string url = 1;
   optional bytes favicon = 2;
   optional string title = 3;
-  // Corresponds to BookmarkNode::date_added() and is the internal value from
-  // base::Time.
+  // Corresponds to BookmarkNode::date_added() represented as microseconds since
+  // the Windows epoch.
   optional int64 creation_time_us = 4;
   optional string icon_url = 5;
   repeated MetaInfo meta_info = 6;

components/sync_bookmarks/BUILD.gn

@@ -16,6 +16,8 @@ static_library("sync_bookmarks") {
     "bookmark_model_type_controller.h",
     "bookmark_model_type_processor.cc",
     "bookmark_model_type_processor.h",
+    "synced_bookmark_tracker.cc",
+    "synced_bookmark_tracker.h",
   ]
 
   deps = [
@@ -36,6 +38,7 @@ source_set("unit_tests") {
     "bookmark_data_type_controller_unittest.cc",
     "bookmark_model_type_controller_unittest.cc",
     "bookmark_model_type_processor_unittest.cc",
+    "synced_bookmark_tracker_unittest.cc",
   ]
 
   deps = [

components/sync_bookmarks/bookmark_model_type_controller.cc

@@ -68,12 +68,15 @@ void BookmarkModelTypeController::RegisterWithBackend(
     base::Callback<void(bool)> set_downloaded,
     syncer::ModelTypeConfigurer* configurer) {
   DCHECK(CalledOnValidThread());
+  if (activated_)
+    return;
   DCHECK(configurer);
   std::unique_ptr<syncer::ActivationContext> activation_context =
       PrepareActivationContext();
   set_downloaded.Run(activation_context->model_type_state.initial_sync_done());
   configurer->ActivateNonBlockingDataType(type(),
                                           std::move(activation_context));
+  activated_ = true;
 }
 
 void BookmarkModelTypeController::StartAssociating(
@@ -92,13 +95,17 @@ void BookmarkModelTypeController::StartAssociating(
 void BookmarkModelTypeController::ActivateDataType(
     syncer::ModelTypeConfigurer* configurer) {
   DCHECK(CalledOnValidThread());
-  NOTIMPLEMENTED();
+  DCHECK(configurer);
+  DCHECK_EQ(RUNNING, state_);
 }
 
 void BookmarkModelTypeController::DeactivateDataType(
     syncer::ModelTypeConfigurer* configurer) {
   DCHECK(CalledOnValidThread());
-  NOTIMPLEMENTED();
+  if (activated_) {
+    configurer->DeactivateNonBlockingDataType(type());
+    activated_ = false;
+  }
 }
 
 void BookmarkModelTypeController::Stop() {
@@ -158,7 +165,8 @@ BookmarkModelTypeController::PrepareActivationContext() {
       directory->InitialSyncEndedForType(type()));
   // TODO(pavely): Populate model_type_state.type_context.
 
-  model_type_processor_ = std::make_unique<BookmarkModelTypeProcessor>();
+  model_type_processor_ =
+      std::make_unique<BookmarkModelTypeProcessor>(sync_client_);
   activation_context->type_processor =
       std::make_unique<syncer::ModelTypeProcessorProxy>(
           model_type_processor_->GetWeakPtr(),

components/sync_bookmarks/bookmark_model_type_controller.h

@@ -61,6 +61,12 @@ class BookmarkModelTypeController : public syncer::DataTypeController {
   // BookmarkModel/HistoryService.
   std::unique_ptr<BookmarkModelTypeProcessor> model_type_processor_;
 
+  // This is a hack to prevent reconfigurations from crashing, because USS
+  // activation is not idempotent. RegisterWithBackend only needs to actually do
+  // something the first time after the type is enabled.
+  // TODO(crbug.com/647505): Remove this once the DTM handles things better.
+  bool activated_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelTypeController);
 };
 

components/sync_bookmarks/bookmark_model_type_processor.h

@@ -6,17 +6,28 @@
 #define COMPONENTS_SYNC_BOOKMARKS_BOOKMARK_MODEL_TYPE_PROCESSOR_H_
 
 #include <memory>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
 #include "components/sync/engine/model_type_processor.h"
+#include "components/sync_bookmarks/synced_bookmark_tracker.h"
+
+namespace syncer {
+class SyncClient;
+}
+
+namespace bookmarks {
+class BookmarkModel;
+}
 
 namespace sync_bookmarks {
 
 class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor {
  public:
-  BookmarkModelTypeProcessor();
+  // |sync_client| must not be nullptr and must outlive this object.
+  explicit BookmarkModelTypeProcessor(syncer::SyncClient* sync_client);
   ~BookmarkModelTypeProcessor() override;
 
   // ModelTypeProcessor implementation.
@@ -30,13 +41,54 @@ class BookmarkModelTypeProcessor : public syncer::ModelTypeProcessor {
   void OnUpdateReceived(const sync_pb::ModelTypeState& type_state,
                         const syncer::UpdateResponseDataList& updates) override;
 
+  // Public for testing.
+  static std::vector<const syncer::UpdateResponseData*> ReorderUpdatesForTest(
+      const syncer::UpdateResponseDataList& updates);
+
   base::WeakPtr<syncer::ModelTypeProcessor> GetWeakPtr();
 
  private:
   SEQUENCE_CHECKER(sequence_checker_);
 
+  // Reorder incoming updates such that parent creation is before child creation
+  // and child deletion is before parent deletion. The returned pointers point
+  // to the elements in the original |updates|.
+  static std::vector<const syncer::UpdateResponseData*> ReorderUpdates(
+      const syncer::UpdateResponseDataList& updates);
+
+  // Given a remote update entity, it returns the parent bookmark node of the
+  // corresponding node. It returns null if the parent node cannot be found.
+  const bookmarks::BookmarkNode* GetParentNode(
+      const syncer::EntityData& update_data) const;
+
+  // Processor a remote creation of a bookmark node.
+  // 1. For permanent folders, they are only registered in |bookmark_tracker_|.
+  // 2. If the nodes parent cannot be found, the remote creation update is
+  //    ignored.
+  // 3. Otherwise, a new node is created in the local bookmark model and
+  //    registered in |bookmark_tracker_|.
+  void ProcessRemoteCreate(const syncer::EntityData& update_data);
+
+  // Associates the permanent bookmark folders with the corresponding server
+  // side ids and registers the association in |bookmark_tracker_|.
+  // |update_data| must contain server_defined_unique_tag that is used to
+  // determine the corresponding permanent node. All permanent nodes are assumed
+  // to be directly children nodes of |kBookmarksRootId|. This method is used in
+  // the initial sync cycle only.
+  void AssociatePermanentFolder(const syncer::EntityData& update_data);
+
+  syncer::SyncClient* const sync_client_;
+
+  // The bookmark model we are processing local changes from and forwarding
+  // remote changes to.
+  bookmarks::BookmarkModel* const bookmark_model_;
+
   std::unique_ptr<syncer::CommitQueue> worker_;
 
+  // Keep the mapping between server ids and bookmarks nodes. It also caches the
+  // metadata upon a local change until the commit configration is received.
+  SyncedBookmarkTracker bookmark_tracker_;
+
   base::WeakPtrFactory<BookmarkModelTypeProcessor> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(BookmarkModelTypeProcessor);

components/sync_bookmarks/bookmark_model_type_processor_unittest.cc

@@ -4,16 +4,141 @@
 
 #include "components/sync_bookmarks/bookmark_model_type_processor.h"
 
+#include <string>
+
+#include "base/strings/utf_string_conversions.h"
+#include "components/bookmarks/browser/bookmark_model.h"
+#include "components/bookmarks/test/test_bookmark_client.h"
+#include "components/sync/driver/fake_sync_client.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::ASCIIToUTF16;
+using testing::Eq;
+using testing::NotNull;
+
 namespace sync_bookmarks {
 
 namespace {
 
-class BookmarkModelTypeProcessorTest : public testing::Test {};
+// The parent tag for children of the root entity. Entities with this parent are
+// referred to as top level enities.
+const char kRootParentTag[] = "0";
+const char kBookmarkBarTag[] = "bookmark_bar";
+const char kBookmarksRootId[] = "32904_google_chrome_bookmarks";
+
+syncer::UpdateResponseData CreateUpdateData(const std::string& id,
+                                            const std::string& title,
+                                            const std::string& url,
+                                            const std::string& parent_id,
+                                            const std::string& server_tag) {
+  syncer::EntityData data;
+  data.id = id;
+  data.parent_id = parent_id;
+  data.server_defined_unique_tag = server_tag;
+
+  sync_pb::BookmarkSpecifics* bookmark_specifics =
+      data.specifics.mutable_bookmark();
+  bookmark_specifics->set_title(title);
+  bookmark_specifics->set_url(url);
+
+  syncer::UpdateResponseData response_data;
+  response_data.entity = data.PassToPtr();
+  // Similar to what's done in the loopback_server.
+  response_data.response_version = 0;
+  return response_data;
+}
+
+syncer::UpdateResponseData CreateBookmarkRootUpdateData() {
+  return CreateUpdateData(syncer::ModelTypeToRootTag(syncer::BOOKMARKS),
+                          std::string(), std::string(), kRootParentTag,
+                          syncer::ModelTypeToRootTag(syncer::BOOKMARKS));
+}
+
+class TestSyncClient : public syncer::FakeSyncClient {
+ public:
+  explicit TestSyncClient(bookmarks::BookmarkModel* bookmark_model)
+      : bookmark_model_(bookmark_model) {}
+
+  bookmarks::BookmarkModel* GetBookmarkModel() override {
+    return bookmark_model_;
+  }
+
+ private:
+  bookmarks::BookmarkModel* bookmark_model_;
+};
+
+class BookmarkModelTypeProcessorTest : public testing::Test {
+ public:
+  BookmarkModelTypeProcessorTest()
+      : bookmark_model_(bookmarks::TestBookmarkClient::CreateModel()),
+        sync_client_(bookmark_model_.get()) {}
+
+  TestSyncClient* sync_client() { return &sync_client_; }
+  bookmarks::BookmarkModel* bookmark_model() { return bookmark_model_.get(); }
+
+ private:
+  std::unique_ptr<bookmarks::BookmarkModel> bookmark_model_;
+  TestSyncClient sync_client_;
+};
+
+TEST_F(BookmarkModelTypeProcessorTest, ReorderUpdatesShouldIgnoreRootNodes) {
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateBookmarkRootUpdateData());
+  std::vector<const syncer::UpdateResponseData*> ordered_updates =
+      BookmarkModelTypeProcessor::ReorderUpdatesForTest(updates);
+  // Root node update should be filtered out.
+  EXPECT_THAT(ordered_updates.size(), Eq(0U));
+}
+
+// TODO(crbug.com/516866): This should change to cover the general case of
+// testing parents before children for non-deletions.
+TEST_F(BookmarkModelTypeProcessorTest,
+       ReorderUpdatesShouldPlacePermanentNodesFirstForNonDeletions) {
+  std::string node1_id = "node1";
+  std::string node2_id = "node2";
+  syncer::UpdateResponseDataList updates;
+  updates.push_back(CreateUpdateData(node1_id, std::string(), std::string(),
+                                     node2_id, std::string()));
+  updates.push_back(CreateUpdateData(node2_id, std::string(), std::string(),
+                                     kBookmarksRootId, kBookmarkBarTag));
+  std::vector<const syncer::UpdateResponseData*> ordered_updates =
+      BookmarkModelTypeProcessor::ReorderUpdatesForTest(updates);
+
+  // No update should be dropped.
+  ASSERT_THAT(ordered_updates.size(), Eq(2U));
+
+  // Updates should be ordered such that parent node update comes first.
+  EXPECT_THAT(ordered_updates[0]->entity.value().id, Eq(node2_id));
+  EXPECT_THAT(ordered_updates[1]->entity.value().id, Eq(node1_id));
+}
+
+TEST_F(BookmarkModelTypeProcessorTest, ShouldUpdateModelAfterRemoteCreation) {
+  BookmarkModelTypeProcessor processor(sync_client());
+
+  syncer::UpdateResponseDataList updates;
+  // Add update for the permanent folder "Bookmarks bar"
+  updates.push_back(CreateUpdateData("bookmark_bar", std::string(),
+                                     std::string(), kBookmarksRootId,
+                                     kBookmarkBarTag));
+
+  // Add update for another node under the bookmarks bar.
+  std::string kNodeId = "node_id";
+  std::string kTitle = "title";
+  std::string kUrl = "http://www.url.com";
+  updates.push_back(CreateUpdateData(kNodeId, kTitle, kUrl, kBookmarkBarTag,
+                                     /*server_tag=*/std::string()));
+
+  const bookmarks::BookmarkNode* bookmarkbar =
+      bookmark_model()->bookmark_bar_node();
+  EXPECT_TRUE(bookmarkbar->empty());
+
+  const sync_pb::ModelTypeState model_type_state;
+  processor.OnUpdateReceived(model_type_state, updates);
 
-TEST_F(BookmarkModelTypeProcessorTest, CreateInstance) {
-  BookmarkModelTypeProcessor processor;
+  ASSERT_THAT(bookmarkbar->GetChild(0), NotNull());
+  EXPECT_THAT(bookmarkbar->GetChild(0)->GetTitle(), Eq(ASCIIToUTF16(kTitle)));
+  EXPECT_THAT(bookmarkbar->GetChild(0)->url(), Eq(GURL(kUrl)));
 }
 
 }  // namespace

components/sync_bookmarks/synced_bookmark_tracker.cc

+// Copyright 2018 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 "components/sync_bookmarks/synced_bookmark_tracker.h"
+
+#include <utility>
+
+#include "components/sync/model/entity_data.h"
+
+namespace sync_bookmarks {
+
+SyncedBookmarkTracker::Entity::Entity(
+    const bookmarks::BookmarkNode* bookmark_node)
+    : bookmark_node_(bookmark_node) {
+  DCHECK(bookmark_node);
+}
+
+SyncedBookmarkTracker::Entity::~Entity() = default;
+
+bool SyncedBookmarkTracker::Entity::MatchesData(
+    const syncer::EntityData& data) const {
+  // TODO(crbug.com/516866): Implement properly.
+  return false;
+}
+
+bool SyncedBookmarkTracker::Entity::IsUnsynced() const {
+  // TODO(crbug.com/516866): Implement properly.
+  return false;
+}
+
+SyncedBookmarkTracker::SyncedBookmarkTracker() = default;
+SyncedBookmarkTracker::~SyncedBookmarkTracker() = default;
+
+const SyncedBookmarkTracker::Entity* SyncedBookmarkTracker::GetEntityForSyncId(
+    const std::string& sync_id) const {
+  auto it = sync_id_to_entities_map_.find(sync_id);
+  return it != sync_id_to_entities_map_.end() ? it->second.get() : nullptr;
+}
+
+void SyncedBookmarkTracker::Associate(
+    const std::string& sync_id,
+    const bookmarks::BookmarkNode* bookmark_node) {
+  sync_id_to_entities_map_[sync_id] = std::make_unique<Entity>(bookmark_node);
+}
+
+}  // namespace sync_bookmarks

components/sync_bookmarks/synced_bookmark_tracker.h

+// Copyright 2018 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.
+
+#ifndef COMPONENTS_SYNC_BOOKMARKS_SYNCED_BOOKMARK_TRACKER_H_
+#define COMPONENTS_SYNC_BOOKMARKS_SYNCED_BOOKMARK_TRACKER_H_
+
+#include <map>
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+#include "components/sync/protocol/entity_metadata.pb.h"
+
+namespace bookmarks {
+class BookmarkNode;
+}
+
+namespace syncer {
+struct EntityData;
+}
+
+namespace sync_bookmarks {
+
+// This class is responsible for keeping the mapping between bookmarks node in
+// the local model and the server-side corresponding sync entities. It manages
+// the metadata for its entity and caches entity data upon a local change until
+// commit confirmation is received.
+class SyncedBookmarkTracker {
+ public:
+  class Entity {
+   public:
+    // |bookmark_node| must not be null and must outlive this object.
+    explicit Entity(const bookmarks::BookmarkNode* bookmark_node);
+    ~Entity();
+
+    // Returns true if this data is out of sync with the server.
+    // A commit may or may not be in progress at this time.
+    bool IsUnsynced() const;
+
+    // Check whether |data| matches the stored specifics hash.
+    bool MatchesData(const syncer::EntityData& data) const;
+
+    // It never returns null.
+    const bookmarks::BookmarkNode* bookmark_node() const {
+      return bookmark_node_;
+    }
+
+   private:
+    const bookmarks::BookmarkNode* const bookmark_node_;
+
+    DISALLOW_COPY_AND_ASSIGN(Entity);
+  };
+
+  SyncedBookmarkTracker();
+  ~SyncedBookmarkTracker();
+
+  // Returns null if not entity is found.
+  const Entity* GetEntityForSyncId(const std::string& sync_id) const;
+
+  // Associates a server id with the corresponding local bookmark node in
+  // |sync_id_to_entities_map_|.
+  void Associate(const std::string& sync_id,
+                 const bookmarks::BookmarkNode* bookmark_node);
+
+ private:
+  // A map of sync server ids to sync entities. This should contain entries and
+  // metadata for almost everything. However, since local data are loaded only
+  // when needed (e.g. before a commit cycle), the entities may not always
+  // contain model type data/specifics.
+  std::map<std::string, std::unique_ptr<Entity>> sync_id_to_entities_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(SyncedBookmarkTracker);
+};
+
+}  // namespace sync_bookmarks
+
+#endif  // COMPONENTS_SYNC_BOOKMARKS_SYNCED_BOOKMARK_TRACKER_H_

components/sync_bookmarks/synced_bookmark_tracker_unittest.cc

+// Copyright 2018 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 "components/sync_bookmarks/synced_bookmark_tracker.h"
+
+#include "components/bookmarks/browser/bookmark_node.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using testing::Eq;
+using testing::IsNull;
+using testing::NotNull;
+
+namespace sync_bookmarks {
+
+namespace {
+
+TEST(SyncedBookmarkTrackerTest, ShouldGetAssociatedNodes) {
+  SyncedBookmarkTracker tracker;
+  const std::string kSyncId = "SYNC_ID";
+  const int64_t kId = 1;
+  bookmarks::BookmarkNode node(kId, GURL());
+  tracker.Associate(kSyncId, &node);
+  const SyncedBookmarkTracker::Entity* entity =
+      tracker.GetEntityForSyncId(kSyncId);
+  ASSERT_THAT(entity, NotNull());
+  EXPECT_THAT(entity->bookmark_node(), Eq(&node));
+  EXPECT_THAT(tracker.GetEntityForSyncId("unknown id"), IsNull());
+}
+
+}  // namespace
+
+}  // namespace sync_bookmarks