[sync] Delete legacy concept of routing info

All datatypes (except proxy types) are non-blocking, so there is little
point in maintaining code that keeps track of which precise "group" each
datatype maps to.

In this patch, most code is deleted except for the internals of
SyncBackendRegistrar, which is more complicated.

Change-Id: I4ad2d219e8032cbb0dca16125958c6e66c117cf6
Bug: 1102835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2467918
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#816676}

chrome/browser/ui/webui/sync_internals_browsertest.js

@@ -205,7 +205,6 @@ HARD_CODED_ABOUT_INFO = {
       'num_live': 'Live Entries',
       'message': 'Message',
       'state': 'State',
-      'group_type': 'Group Type',
     },
     {
       'status': 'ok',
@@ -214,7 +213,6 @@ HARD_CODED_ABOUT_INFO = {
       'num_live': 2793,
       'message': '',
       'state': 'Running',
-      'group_type': 'Group UI',
     },
   ],
   'unrecoverable_error_detected': false

components/sync/driver/glue/sync_engine_backend.cc

@@ -143,8 +143,6 @@ void SyncEngineBackend::OnInitializationComplete(
       /*types_to_add=*/ControlTypes(),
       /*types_to_remove=*/ModelTypeSet());
 
-  ModelSafeRoutingInfo routing_info;
-  registrar_->GetModelSafeRoutingInfo(&routing_info);
   SDVLOG(1) << "Control Types " << ModelTypeSetToString(new_control_types)
             << " added; calling ConfigureSyncer";
 
@@ -469,9 +467,7 @@ void SyncEngineBackend::DoFinishConfigureDataTypes(
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // Update the enabled types for the bridge and sync manager.
-  ModelSafeRoutingInfo routing_info;
-  registrar_->GetModelSafeRoutingInfo(&routing_info);
-  ModelTypeSet enabled_types = GetRoutingInfoTypes(routing_info);
+  ModelTypeSet enabled_types = registrar_->GetTypesWithRoutingInfo();
   enabled_types.RemoveAll(ProxyTypes());
 
   const ModelTypeSet failed_configuration_types =

components/sync/driver/glue/sync_engine_impl.cc

@@ -263,14 +263,6 @@ void SyncEngineImpl::HasUnsyncedItemsForTest(
       std::move(cb));
 }
 
-void SyncEngineImpl::GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const {
-  if (IsInitialized()) {
-    registrar_->GetModelSafeRoutingInfo(out);
-  } else {
-    NOTREACHED();
-  }
-}
-
 void SyncEngineImpl::RequestBufferedProtocolEventsAndEnableForwarding() {
   sync_task_runner_->PostTask(
       FROM_HERE,

components/sync/driver/glue/sync_engine_impl.h

@@ -83,7 +83,6 @@ class SyncEngineImpl : public SyncEngine,
   const Status& GetDetailedStatus() const override;
   void HasUnsyncedItemsForTest(
       base::OnceCallback<void(bool)> cb) const override;
-  void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const override;
   void RequestBufferedProtocolEventsAndEnableForwarding() override;
   void DisableProtocolEventForwarding() override;
   void OnCookieJarChanged(bool account_mismatch,

components/sync/driver/profile_sync_service.cc

@@ -1483,17 +1483,12 @@ ProfileSyncService::GetTypeStatusMapForDebugging() {
   type_status_header->SetString("num_live", "Live Entries");
   type_status_header->SetString("message", "Message");
   type_status_header->SetString("state", "State");
-  type_status_header->SetString("group_type", "Group Type");
   result->Append(std::move(type_status_header));
 
-  ModelSafeRoutingInfo routing_info;
-  engine_->GetModelSafeRoutingInfo(&routing_info);
   const ModelTypeSet registered = GetRegisteredDataTypes();
   for (ModelType type : registered) {
     auto type_status = std::make_unique<base::DictionaryValue>();
     type_status->SetString("name", ModelTypeToString(type));
-    type_status->SetString("group_type",
-                           ModelSafeGroupToString(routing_info[type]));
 
     if (data_type_error_map_.find(type) != data_type_error_map_.end()) {
       const SyncError& error = data_type_error_map_.find(type)->second;
@@ -1516,12 +1511,9 @@ ProfileSyncService::GetTypeStatusMapForDebugging() {
     } else if (backed_off_types.Has(type)) {
       type_status->SetString("status", "warning");
       type_status->SetString("message", "Backed off");
-    } else if (routing_info.find(type) != routing_info.end()) {
+    } else {
       type_status->SetString("status", "ok");
       type_status->SetString("message", "");
-    } else {
-      type_status->SetString("status", "warning");
-      type_status->SetString("message", "Disabled by User");
     }
 
     const auto& dtc_iter = data_type_controllers_.find(type);

components/sync/driver/resources/about.html

@@ -55,9 +55,8 @@
         <td jscontent="name" width=30%></td>
         <td jscontent="num_entries" width=10%></td>
         <td jscontent="num_live" width=10%></td>
-        <td jscontent="message" width=30%></td>
+        <td jscontent="message" width=40%></td>
         <td jscontent="state" width=10%></td>
-        <td jscontent="group_type" width=10%></td>
       </tr>
     </table>
   </div>

components/sync/engine/fake_sync_engine.cc

@@ -75,8 +75,6 @@ const SyncStatus& FakeSyncEngine::GetDetailedStatus() const {
 void FakeSyncEngine::HasUnsyncedItemsForTest(
     base::OnceCallback<void(bool)> cb) const {}
 
-void FakeSyncEngine::GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const {}
-
 void FakeSyncEngine::RequestBufferedProtocolEventsAndEnableForwarding() {}
 
 void FakeSyncEngine::DisableProtocolEventForwarding() {}

components/sync/engine/fake_sync_engine.h

@@ -72,8 +72,6 @@ class FakeSyncEngine : public SyncEngine {
   void HasUnsyncedItemsForTest(
       base::OnceCallback<void(bool)> cb) const override;
 
-  void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const override;
-
   void RequestBufferedProtocolEventsAndEnableForwarding() override;
   void DisableProtocolEventForwarding() override;
 

components/sync/engine/mock_sync_engine.h

@@ -51,7 +51,6 @@ class MockSyncEngine : public SyncEngine {
   MOCK_CONST_METHOD0(GetDetailedStatus, const SyncStatus&());
   MOCK_CONST_METHOD1(HasUnsyncedItemsForTest,
                      void(base::OnceCallback<void(bool)>));
-  MOCK_CONST_METHOD1(GetModelSafeRoutingInfo, void(ModelSafeRoutingInfo*));
   MOCK_METHOD0(RequestBufferedProtocolEventsAndEnableForwarding, void());
   MOCK_METHOD0(DisableProtocolEventForwarding, void());
   MOCK_METHOD1(ClearServerData, void(base::OnceClosure));

components/sync/engine/model_safe_worker.cc

@@ -7,63 +7,15 @@
 #include <utility>
 
 #include "base/bind.h"
-#include "base/json/json_writer.h"
 #include "base/threading/thread_restrictions.h"
-#include "base/values.h"
 
 namespace syncer {
 
-std::unique_ptr<base::DictionaryValue> ModelSafeRoutingInfoToValue(
-    const ModelSafeRoutingInfo& routing_info) {
-  std::unique_ptr<base::DictionaryValue> dict(new base::DictionaryValue());
-  for (auto it = routing_info.begin(); it != routing_info.end(); ++it) {
-    dict->SetString(ModelTypeToString(it->first),
-                    ModelSafeGroupToString(it->second));
-  }
-  return dict;
-}
-
-std::string ModelSafeRoutingInfoToString(
-    const ModelSafeRoutingInfo& routing_info) {
-  std::string json;
-  base::JSONWriter::Write(*ModelSafeRoutingInfoToValue(routing_info), &json);
-  return json;
-}
-
-ModelTypeSet GetRoutingInfoTypes(const ModelSafeRoutingInfo& routing_info) {
-  ModelTypeSet types;
-  for (auto it = routing_info.begin(); it != routing_info.end(); ++it) {
-    types.Put(it->first);
-  }
-  return types;
-}
-
-ModelSafeGroup GetGroupForModelType(const ModelType type,
-                                    const ModelSafeRoutingInfo& routes) {
-  auto it = routes.find(type);
-  if (it == routes.end()) {
-    if (type != UNSPECIFIED && type != TOP_LEVEL_FOLDER)
-      DVLOG(1) << "Entry does not belong to active ModelSafeGroup!";
-    return GROUP_PASSIVE;
-  }
-  return it->second;
-}
-
-std::string ModelSafeGroupToString(ModelSafeGroup group) {
-  switch (group) {
-    case GROUP_PASSIVE:
-      return "Group Passive";
-    case GROUP_NON_BLOCKING:
-      return "Group Non Blocking";
-  }
-  NOTREACHED();
-  return "Invalid";
-}
-
 ModelSafeWorker::ModelSafeWorker()
     : work_done_or_abandoned_(base::WaitableEvent::ResetPolicy::AUTOMATIC,
                               base::WaitableEvent::InitialState::NOT_SIGNALED) {
 }
+
 ModelSafeWorker::~ModelSafeWorker() {}
 
 void ModelSafeWorker::RequestStop() {

components/sync/engine/model_safe_worker.h

@@ -18,10 +18,6 @@
 #include "components/sync/base/model_type.h"
 #include "components/sync/base/syncer_error.h"
 
-namespace base {
-class DictionaryValue;
-}  // namespace base
-
 namespace syncer {
 
 using WorkCallback = base::OnceCallback<SyncerError(void)>;
@@ -38,8 +34,6 @@ enum ModelSafeGroup {
                        // SyncBackendRegistrar involvement.
 };
 
-std::string ModelSafeGroupToString(ModelSafeGroup group);
-
 // TODO(crbug.com/1102835): This class is a remainder from the old Directory
 // implementation and should be removed.
 //
@@ -99,23 +93,6 @@ class ModelSafeWorker : public base::RefCountedThreadSafe<ModelSafeWorker> {
   DISALLOW_COPY_AND_ASSIGN(ModelSafeWorker);
 };
 
-// A map that details which ModelSafeGroup each ModelType
-// belongs to.  Routing info can change in response to the user enabling /
-// disabling sync for certain types, as well as model association completions.
-using ModelSafeRoutingInfo = std::map<ModelType, ModelSafeGroup>;
-
-// Caller takes ownership of return value.
-std::unique_ptr<base::DictionaryValue> ModelSafeRoutingInfoToValue(
-    const ModelSafeRoutingInfo& routing_info);
-
-std::string ModelSafeRoutingInfoToString(
-    const ModelSafeRoutingInfo& routing_info);
-
-ModelTypeSet GetRoutingInfoTypes(const ModelSafeRoutingInfo& routing_info);
-
-ModelSafeGroup GetGroupForModelType(const ModelType type,
-                                    const ModelSafeRoutingInfo& routes);
-
 }  // namespace syncer
 
 #endif  // COMPONENTS_SYNC_ENGINE_MODEL_SAFE_WORKER_H_

components/sync/engine/model_safe_worker_unittest.cc

@@ -84,34 +84,6 @@ class ModelSafeWorkerTest : public ::testing::Test {
 
 }  // namespace
 
-TEST_F(ModelSafeWorkerTest, ModelSafeRoutingInfoToValue) {
-  ModelSafeRoutingInfo routing_info;
-  routing_info[BOOKMARKS] = GROUP_PASSIVE;
-  routing_info[APPS] = GROUP_NON_BLOCKING;
-  base::DictionaryValue expected_value;
-  expected_value.SetString("Apps", "Group Non Blocking");
-  expected_value.SetString("Bookmarks", "Group Passive");
-  std::unique_ptr<base::DictionaryValue> value(
-      ModelSafeRoutingInfoToValue(routing_info));
-  EXPECT_TRUE(value->Equals(&expected_value));
-}
-
-TEST_F(ModelSafeWorkerTest, ModelSafeRoutingInfoToString) {
-  ModelSafeRoutingInfo routing_info;
-  routing_info[APPS] = GROUP_NON_BLOCKING;
-  routing_info[BOOKMARKS] = GROUP_PASSIVE;
-  EXPECT_EQ("{\"Apps\":\"Group Non Blocking\",\"Bookmarks\":\"Group Passive\"}",
-            ModelSafeRoutingInfoToString(routing_info));
-}
-
-TEST_F(ModelSafeWorkerTest, GetRoutingInfoTypes) {
-  ModelSafeRoutingInfo routing_info;
-  routing_info[BOOKMARKS] = GROUP_PASSIVE;
-  routing_info[PASSWORDS] = GROUP_NON_BLOCKING;
-  const ModelTypeSet expected_types(BOOKMARKS, PASSWORDS);
-  EXPECT_EQ(expected_types, GetRoutingInfoTypes(routing_info));
-}
-
 TEST_F(ModelSafeWorkerTest, DoWorkAndWaitUntilDone) {
   bool did_work = false;
   DoWorkAndWaitUntilDoneOnSyncThread(base::BindOnce(

components/sync/engine/sync_backend_registrar.cc

@@ -57,7 +57,7 @@ void SyncBackendRegistrar::SetInitialTypes(ModelTypeSet initial_types) {
 
   // Although this can re-set types in GROUP_NON_BLOCKING, this should be
   // idempotent.
-  last_configured_types_ = GetRoutingInfoTypes(routing_info_);
+  last_configured_types_ = GetTypesWithRoutingInfoNoLock();
 }
 
 void SyncBackendRegistrar::AddRestoredDataType(ModelType type) {
@@ -96,14 +96,11 @@ ModelTypeSet SyncBackendRegistrar::ConfigureDataTypes(
     routing_info_.erase(type);
   }
 
-  // TODO(akalin): Use SVLOG/SLOG if we add any more logging.
   DVLOG(1) << name_ << ": Adding types " << ModelTypeSetToString(types_to_add)
            << " (with newly-added types "
            << ModelTypeSetToString(newly_added_types) << ") and removing types "
-           << ModelTypeSetToString(types_to_remove)
-           << " to get new routing info "
-           << ModelSafeRoutingInfoToString(routing_info_);
-  last_configured_types_ = GetRoutingInfoTypes(routing_info_);
+           << ModelTypeSetToString(types_to_remove);
+  last_configured_types_ = GetTypesWithRoutingInfoNoLock();
 
   return newly_added_types;
 }
@@ -129,25 +126,17 @@ void SyncBackendRegistrar::GetWorkers(
   }
 }
 
-void SyncBackendRegistrar::GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) {
+ModelTypeSet SyncBackendRegistrar::GetTypesWithRoutingInfo() const {
   base::AutoLock lock(lock_);
-  ModelSafeRoutingInfo copy(routing_info_);
-  out->swap(copy);
+  return GetTypesWithRoutingInfoNoLock();
 }
 
-bool SyncBackendRegistrar::IsCurrentThreadSafeForModel(
-    ModelType model_type) const {
-  lock_.AssertAcquired();
-  ModelSafeGroup group = GetGroupForModelType(model_type, routing_info_);
-  DCHECK_NE(GROUP_NON_BLOCKING, group);
-
-  if (group == GROUP_PASSIVE) {
-    return IsControlType(model_type);
+ModelTypeSet SyncBackendRegistrar::GetTypesWithRoutingInfoNoLock() const {
+  ModelTypeSet types;
+  for (const auto& model_type_and_group : routing_info_) {
+    types.Put(model_type_and_group.first);
   }
-
-  auto it = workers_.find(group);
-  DCHECK(it != workers_.end());
-  return it->second->IsOnModelSequence();
+  return types;
 }
 
 SyncBackendRegistrar::~SyncBackendRegistrar() {

components/sync/engine/sync_backend_registrar.h

@@ -83,18 +83,20 @@ class SyncBackendRegistrar {
   void RequestWorkerStopOnUIThread();
 
   void GetWorkers(std::vector<scoped_refptr<ModelSafeWorker>>* out);
-  void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out);
+
+  // Returns the set of currently enabled types.
+  ModelTypeSet GetTypesWithRoutingInfo() const;
 
  private:
+  // Same as GetTypesWithRoutingInfo() but callers are responsible for holding
+  // |lock_|.
+  ModelTypeSet GetTypesWithRoutingInfoNoLock() const;
+
   // Add a worker for |group| to the worker map if one is successfully created
   // by |worker_factory|.
   void MaybeAddWorker(ModelSafeWorkerFactory worker_factory,
                       ModelSafeGroup group);
 
-  // Return true if |model_type| lives on the current thread.  Must be
-  // called with |lock_| held.  May be called on any thread.
-  bool IsCurrentThreadSafeForModel(ModelType model_type) const;
-
   // Returns model safe group that should be assigned to type when it is first
   // configured (before activation).
   ModelSafeGroup GetInitialGroupForType(ModelType type) const;
@@ -112,7 +114,7 @@ class SyncBackendRegistrar {
   std::map<ModelSafeGroup, scoped_refptr<ModelSafeWorker>> workers_;
 
   // Maps ModelType to ModelSafeGroup.
-  ModelSafeRoutingInfo routing_info_;
+  std::map<ModelType, ModelSafeGroup> routing_info_;
 
   // The types that were enabled as of the last configuration. Updated on each
   // call to ConfigureDataTypes as well as SetInitialTypes.

components/sync/engine/sync_backend_registrar_unittest.cc

@@ -39,10 +39,9 @@ class SyncBackendRegistrarTest : public testing::Test {
     sync_thread_.FlushForTesting();
   }
 
-  void ExpectRoutingInfo(const ModelSafeRoutingInfo& expected_routing_info) {
-    ModelSafeRoutingInfo actual_routing_info;
-    registrar_->GetModelSafeRoutingInfo(&actual_routing_info);
-    EXPECT_EQ(expected_routing_info, actual_routing_info);
+  void ExpectRoutingInfo(ModelTypeSet expected_routing_info_types) {
+    EXPECT_EQ(expected_routing_info_types,
+              registrar_->GetTypesWithRoutingInfo());
   }
 
   size_t GetWorkersSize() {
@@ -78,7 +77,7 @@ TEST_F(SyncBackendRegistrarTest, ConstructorEmpty) {
   registrar()->SetInitialTypes(ModelTypeSet());
   EXPECT_FALSE(registrar()->IsNigoriEnabled());
   EXPECT_EQ(1u, GetWorkersSize());
-  ExpectRoutingInfo(ModelSafeRoutingInfo());
+  ExpectRoutingInfo(ModelTypeSet());
 }
 
 TEST_F(SyncBackendRegistrarTest, ConstructorNonEmpty) {
@@ -89,7 +88,7 @@ TEST_F(SyncBackendRegistrarTest, ConstructorNonEmpty) {
   EXPECT_EQ(ModelTypeSet(NIGORI), registrar()->GetLastConfiguredTypes());
   // Bookmarks dropped because it is in ModelSafeGroup::GROUP_NON_BLOCKING.
   // Passwords dropped because of no password store.
-  ExpectRoutingInfo({{NIGORI, GROUP_PASSIVE}});
+  ExpectRoutingInfo({NIGORI});
 }
 
 TEST_F(SyncBackendRegistrarTest, ConstructorNonEmptyReversedInitialization) {
@@ -100,7 +99,7 @@ TEST_F(SyncBackendRegistrarTest, ConstructorNonEmptyReversedInitialization) {
   EXPECT_EQ(ModelTypeSet(NIGORI), registrar()->GetLastConfiguredTypes());
   // Bookmarks dropped because it is in ModelSafeGroup::GROUP_NON_BLOCKING.
   // Passwords dropped because of no password store.
-  ExpectRoutingInfo({{NIGORI, GROUP_PASSIVE}});
+  ExpectRoutingInfo({NIGORI});
 }
 
 TEST_F(SyncBackendRegistrarTest, ConfigureDataTypes) {
@@ -110,21 +109,19 @@ TEST_F(SyncBackendRegistrarTest, ConfigureDataTypes) {
   // Add.
   const ModelTypeSet types1(BOOKMARKS, NIGORI, AUTOFILL);
   EXPECT_EQ(types1, registrar()->ConfigureDataTypes(types1, ModelTypeSet()));
-  ExpectRoutingInfo({{BOOKMARKS, GROUP_NON_BLOCKING},
-                     {NIGORI, GROUP_PASSIVE},
-                     {AUTOFILL, GROUP_PASSIVE}});
+  ExpectRoutingInfo({BOOKMARKS, NIGORI, AUTOFILL});
   EXPECT_EQ(types1, registrar()->GetLastConfiguredTypes());
 
   // Add and remove.
   const ModelTypeSet types2(PREFERENCES, THEMES);
   EXPECT_EQ(types2, registrar()->ConfigureDataTypes(types2, types1));
 
-  ExpectRoutingInfo({{PREFERENCES, GROUP_PASSIVE}, {THEMES, GROUP_PASSIVE}});
+  ExpectRoutingInfo({PREFERENCES, THEMES});
   EXPECT_EQ(types2, registrar()->GetLastConfiguredTypes());
 
   // Remove.
   EXPECT_TRUE(registrar()->ConfigureDataTypes(ModelTypeSet(), types2).Empty());
-  ExpectRoutingInfo(ModelSafeRoutingInfo());
+  ExpectRoutingInfo(ModelTypeSet());
   EXPECT_EQ(ModelTypeSet(), registrar()->GetLastConfiguredTypes());
 }
 
@@ -134,12 +131,12 @@ TEST_F(SyncBackendRegistrarTest, ConfigureDataType) {
   registrar()->RegisterDataType(AUTOFILL);
   registrar()->RegisterDataType(BOOKMARKS);
 
-  ExpectRoutingInfo(ModelSafeRoutingInfo());
+  ExpectRoutingInfo(ModelTypeSet());
   // Simulate that initial sync was already done for AUTOFILL.
   registrar()->AddRestoredDataType(AUTOFILL);
   // It should be added to routing info and set of configured types.
   EXPECT_EQ(ModelTypeSet(AUTOFILL), registrar()->GetLastConfiguredTypes());
-  ExpectRoutingInfo({{AUTOFILL, GROUP_NON_BLOCKING}});
+  ExpectRoutingInfo({AUTOFILL});
 
   // Configure two data types. Initial sync wasn't done for BOOKMARKS so
   // it should be included in types to be downloaded.
@@ -148,8 +145,7 @@ TEST_F(SyncBackendRegistrarTest, ConfigureDataType) {
       registrar()->ConfigureDataTypes(types_to_add, ModelTypeSet());
   EXPECT_EQ(ModelTypeSet(BOOKMARKS), newly_added_types);
   EXPECT_EQ(types_to_add, registrar()->GetLastConfiguredTypes());
-  ExpectRoutingInfo(
-      {{AUTOFILL, GROUP_NON_BLOCKING}, {BOOKMARKS, GROUP_NON_BLOCKING}});
+  ExpectRoutingInfo({AUTOFILL, BOOKMARKS});
 }
 
 }  // namespace

components/sync/engine/sync_engine.h

@@ -156,8 +156,6 @@ class SyncEngine : public ModelTypeConfigurer {
       base::OnceCallback<void(bool)> cb) const = 0;
 
 
-  virtual void GetModelSafeRoutingInfo(ModelSafeRoutingInfo* out) const = 0;
-
   // Requests that the backend forward to the fronent any protocol events in
   // its buffer and begin forwarding automatically from now on.  Repeated calls
   // to this function may result in the same events being emitted several