Integrate history delete directive sync in USS

We leverage the recently introduced layer that allows integrating
legacy SyncableService implementations within the newest architecture,
USS. This requires forking the custom controller, because a different
class hierarchy is used.

The new codepath is behind feature toggle
kSyncPseudoUSSHistoryDeleteDirectives.

Bug: 870624
Change-Id: Ief8fb0d5f5d0472aeea1e0edf42e905632bc33fb
Reviewed-on: https://chromium-review.googlesource.com/1251262Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595735}

chrome/browser/sync/test/integration/single_client_history_delete_directives_sync_test.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 <stddef.h>
+
+#include "base/macros.h"
+#include "base/task/cancelable_task_tracker.h"
+#include "base/test/scoped_feature_list.h"
+#include "chrome/browser/history/history_service_factory.h"
+#include "chrome/browser/history/web_history_service_factory.h"
+#include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "chrome/browser/sync/test/integration/sync_test.h"
+#include "chrome/browser/sync/test/integration/updated_progress_marker_checker.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/history/core/browser/history_types.h"
+#include "components/sync/driver/sync_driver_switches.h"
+#include "components/sync/protocol/sync.pb.h"
+#include "components/sync/test/fake_server/fake_server.h"
+
+namespace {
+
+using sync_pb::HistoryDeleteDirectiveSpecifics;
+
+// 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::kSyncPseudoUSSHistoryDeleteDirectives);
+    } else {
+      override_features_.InitAndDisableFeature(
+          switches::kSyncPseudoUSSHistoryDeleteDirectives);
+    }
+  }
+
+ private:
+  base::test::ScopedFeatureList override_features_;
+};
+
+// Allows to wait until the number of server-side entities is equal to a
+// expected number.
+class HistoryDeleteDirectivesEqualityChecker
+    : public SingleClientStatusChangeChecker {
+ public:
+  HistoryDeleteDirectivesEqualityChecker(
+      browser_sync::ProfileSyncService* service,
+      fake_server::FakeServer* fake_server,
+      size_t num_expected_directives)
+      : SingleClientStatusChangeChecker(service),
+        fake_server_(fake_server),
+        num_expected_directives_(num_expected_directives) {}
+
+  bool IsExitConditionSatisfied() override {
+    const std::vector<sync_pb::SyncEntity> entities =
+        fake_server_->GetSyncEntitiesByModelType(
+            syncer::HISTORY_DELETE_DIRECTIVES);
+
+    if (entities.size() == num_expected_directives_) {
+      return true;
+    }
+
+    // |entities.size()| is only going to grow, if |entities.size()| ever
+    // becomes bigger then all hope is lost of passing, stop now.
+    EXPECT_LT(entities.size(), num_expected_directives_)
+        << "Entity set will never become equal";
+    return false;
+  }
+
+  std::string GetDebugMessage() const override {
+    return "Waiting server side HISTORY_DELETE_DIRECTIVES to match expected.";
+  }
+
+ private:
+  fake_server::FakeServer* const fake_server_;
+  const size_t num_expected_directives_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryDeleteDirectivesEqualityChecker);
+};
+
+class SingleClientHistoryDeleteDirectivesSyncTest : public UssSwitchToggler,
+                                                    public SyncTest {
+ public:
+  SingleClientHistoryDeleteDirectivesSyncTest() : SyncTest(SINGLE_CLIENT) {}
+  ~SingleClientHistoryDeleteDirectivesSyncTest() override {}
+
+  bool WaitForHistoryDeleteDirectives(size_t num_expected_directives) {
+    return HistoryDeleteDirectivesEqualityChecker(
+               GetSyncService(0), GetFakeServer(), num_expected_directives)
+        .Wait();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SingleClientHistoryDeleteDirectivesSyncTest);
+};
+
+IN_PROC_BROWSER_TEST_P(SingleClientHistoryDeleteDirectivesSyncTest,
+                       ShouldCommitDeleteDirective) {
+  const GURL kPageUrl = GURL("http://foo.com");
+  const base::Time kHistoryEntryTime = base::Time::Now();
+  base::CancelableTaskTracker task_tracker;
+
+  ASSERT_TRUE(SetupSync());
+
+  history::HistoryService* history_service =
+      HistoryServiceFactory::GetForProfileWithoutCreating(GetProfile(0));
+  history_service->AddPage(kPageUrl, kHistoryEntryTime,
+                           history::SOURCE_BROWSED);
+
+  history_service->ExpireLocalAndRemoteHistoryBetween(
+      WebHistoryServiceFactory::GetForProfile(GetProfile(0)), std::set<GURL>(),
+      /*begin_time=*/base::Time(), /*end_time=*/base::Time(), base::DoNothing(),
+      &task_tracker);
+
+  EXPECT_TRUE(WaitForHistoryDeleteDirectives(1));
+}
+
+INSTANTIATE_TEST_CASE_P(USS,
+                        SingleClientHistoryDeleteDirectivesSyncTest,
+                        ::testing::Values(false, true));
+
+}  // namespace

chrome/test/BUILD.gn

@@ -5349,6 +5349,7 @@ if (!is_android && !is_fuchsia) {
       "../browser/sync/test/integration/single_client_dictionary_sync_test.cc",
       "../browser/sync/test/integration/single_client_directory_sync_test.cc",
       "../browser/sync/test/integration/single_client_extensions_sync_test.cc",
+      "../browser/sync/test/integration/single_client_history_delete_directives_sync_test.cc",
       "../browser/sync/test/integration/single_client_passwords_sync_test.cc",
       "../browser/sync/test/integration/single_client_polling_sync_test.cc",
       "../browser/sync/test/integration/single_client_preferences_sync_test.cc",

components/browser_sync/profile_sync_components_factory_impl.cc

@@ -21,6 +21,7 @@
 #include "components/browser_sync/profile_sync_service.h"
 #include "components/dom_distiller/core/dom_distiller_features.h"
 #include "components/history/core/browser/history_delete_directives_data_type_controller.h"
+#include "components/history/core/browser/history_delete_directives_model_type_controller.h"
 #include "components/history/core/browser/typed_url_model_type_controller.h"
 #include "components/password_manager/core/browser/password_data_type_controller.h"
 #include "components/password_manager/core/browser/password_store.h"
@@ -239,9 +240,17 @@ ProfileSyncComponentsFactoryImpl::CreateCommonDataTypeControllers(
 
     // Delete directive sync is enabled by default.
     if (!disabled_types.Has(syncer::HISTORY_DELETE_DIRECTIVES)) {
-      controllers.push_back(
-          std::make_unique<HistoryDeleteDirectivesDataTypeController>(
-              error_callback, sync_client_));
+      if (base::FeatureList::IsEnabled(
+              switches::kSyncPseudoUSSHistoryDeleteDirectives)) {
+        controllers.push_back(
+            std::make_unique<HistoryDeleteDirectivesModelTypeController>(
+                sync_client_));
+
+      } else {
+        controllers.push_back(
+            std::make_unique<HistoryDeleteDirectivesDataTypeController>(
+                error_callback, sync_client_));
+      }
     }
 
     // Session sync is enabled by default.  This is disabled if history is

components/history/core/browser/BUILD.gn

@@ -40,6 +40,8 @@ static_library("browser") {
     "history_db_task.h",
     "history_delete_directives_data_type_controller.cc",
     "history_delete_directives_data_type_controller.h",
+    "history_delete_directives_model_type_controller.cc",
+    "history_delete_directives_model_type_controller.h",
     "history_model_worker.cc",
     "history_model_worker.h",
     "history_service.cc",

components/history/core/browser/OWNERS

+per-file history_delete_directives_data_type_controller*=file://components/sync/OWNERS
+per-file history_delete_directives_model_type_controller*=file://components/sync/OWNERS
 per-file typed_url_model_type_controller*=file://components/sync/OWNERS
 per-file typed_url_sync_bridge*=file://components/sync/OWNERS

components/history/core/browser/history_delete_directives_data_type_controller.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_HISTORY_CORE_BROWSER_HISTORY_DELETE_DIRECTIVES_DATA_TYPE_CONTROLLER_H_
 
 #include "base/macros.h"
-#include "components/sync/device_info/local_device_info_provider.h"
 #include "components/sync/driver/async_directory_type_controller.h"
 #include "components/sync/driver/sync_service_observer.h"
 

components/history/core/browser/history_delete_directives_model_type_controller.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/history/core/browser/history_delete_directives_model_type_controller.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "components/sync/driver/sync_client.h"
+#include "components/sync/driver/sync_service.h"
+#include "components/sync/model/model_type_store_service.h"
+
+namespace browser_sync {
+
+HistoryDeleteDirectivesModelTypeController::
+    HistoryDeleteDirectivesModelTypeController(syncer::SyncClient* sync_client)
+    : SyncableServiceBasedModelTypeController(
+          syncer::HISTORY_DELETE_DIRECTIVES,
+          sync_client->GetModelTypeStoreService()->GetStoreFactory(),
+          base::BindOnce(&syncer::SyncClient::GetSyncableServiceForType,
+                         base::Unretained(sync_client),
+                         syncer::HISTORY_DELETE_DIRECTIVES)),
+      sync_client_(sync_client) {}
+
+HistoryDeleteDirectivesModelTypeController::
+    ~HistoryDeleteDirectivesModelTypeController() {}
+
+bool HistoryDeleteDirectivesModelTypeController::ReadyForStart() const {
+  DCHECK(CalledOnValidThread());
+  return !sync_client_->GetSyncService()->IsEncryptEverythingEnabled();
+}
+
+void HistoryDeleteDirectivesModelTypeController::LoadModels(
+    const syncer::ConfigureContext& configure_context,
+    const ModelLoadCallback& model_load_callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK_EQ(NOT_RUNNING, state());
+
+  if (DisableTypeIfNecessary()) {
+    return;
+  }
+
+  sync_client_->GetSyncService()->AddObserver(this);
+  SyncableServiceBasedModelTypeController::LoadModels(configure_context,
+                                                      model_load_callback);
+}
+
+void HistoryDeleteDirectivesModelTypeController::Stop(
+    syncer::SyncStopMetadataFate metadata_fate,
+    StopCallback callback) {
+  DCHECK(CalledOnValidThread());
+
+  if (sync_client_->GetSyncService()->HasObserver(this)) {
+    sync_client_->GetSyncService()->RemoveObserver(this);
+  }
+
+  SyncableServiceBasedModelTypeController::Stop(metadata_fate,
+                                                std::move(callback));
+}
+
+void HistoryDeleteDirectivesModelTypeController::OnStateChanged(
+    syncer::SyncService* sync) {
+  DCHECK(CalledOnValidThread());
+  DisableTypeIfNecessary();
+}
+
+bool HistoryDeleteDirectivesModelTypeController::DisableTypeIfNecessary() {
+  DCHECK(CalledOnValidThread());
+
+  if (!sync_client_->GetSyncService()->IsSyncFeatureActive()) {
+    return false;
+  }
+
+  if (ReadyForStart()) {
+    return false;
+  }
+
+  if (sync_client_->GetSyncService()->HasObserver(this)) {
+    sync_client_->GetSyncService()->RemoveObserver(this);
+  }
+
+  ReportModelError(
+      syncer::SyncError::DATATYPE_POLICY_ERROR,
+      syncer::ModelError(FROM_HERE,
+                         "Delete directives not supported with encryption."));
+  return true;
+}
+
+}  // namespace browser_sync

components/history/core/browser/history_delete_directives_model_type_controller.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_HISTORY_CORE_BROWSER_HISTORY_DELETE_DIRECTIVES_MODEL_TYPE_CONTROLLER_H_
+#define COMPONENTS_HISTORY_CORE_BROWSER_HISTORY_DELETE_DIRECTIVES_MODEL_TYPE_CONTROLLER_H_
+
+#include "base/macros.h"
+#include "components/sync/driver/sync_service_observer.h"
+#include "components/sync/driver/syncable_service_based_model_type_controller.h"
+
+namespace syncer {
+class SyncClient;
+}  // namespace syncer
+
+namespace browser_sync {
+
+// A controller for delete directives, which cannot sync when full encryption
+// is enabled.
+class HistoryDeleteDirectivesModelTypeController
+    : public syncer::SyncableServiceBasedModelTypeController,
+      public syncer::SyncServiceObserver {
+ public:
+  // |sync_client| must not be null and must outlive this object.
+  explicit HistoryDeleteDirectivesModelTypeController(
+      syncer::SyncClient* sync_client);
+  ~HistoryDeleteDirectivesModelTypeController() override;
+
+  // DataTypeController overrides.
+  bool ReadyForStart() const override;
+  void LoadModels(const syncer::ConfigureContext& configure_context,
+                  const ModelLoadCallback& model_load_callback) override;
+  void Stop(syncer::SyncStopMetadataFate metadata_fate,
+            StopCallback callback) override;
+
+  // syncer::SyncServiceObserver implementation.
+  void OnStateChanged(syncer::SyncService* sync) override;
+
+ private:
+  // Triggers a SingleDataTypeUnrecoverable error and returns true if the
+  // type is no longer ready, else does nothing and returns false.
+  bool DisableTypeIfNecessary();
+
+  syncer::SyncClient* const sync_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(HistoryDeleteDirectivesModelTypeController);
+};
+
+}  // namespace browser_sync
+
+#endif  // COMPONENTS_HISTORY_CORE_BROWSER_HISTORY_DELETE_DIRECTIVES_MODEL_TYPE_CONTROLLER_H_

components/sync/driver/model_type_controller.cc

@@ -21,14 +21,9 @@
 namespace syncer {
 namespace {
 
-void ReportError(ModelType model_type,
-                 scoped_refptr<base::SequencedTaskRunner> ui_thread,
+void ReportError(scoped_refptr<base::SequencedTaskRunner> ui_thread,
                  const ModelErrorHandler& error_handler,
                  const ModelError& error) {
-  // TODO(wychen): enum uma should be strongly typed. crbug.com/661401
-  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures",
-                            ModelTypeToHistogramInt(model_type),
-                            static_cast<int>(MODEL_TYPE_COUNT));
   ui_thread->PostTask(error.location(), base::BindOnce(error_handler, error));
 }
 
@@ -90,7 +85,7 @@ void ModelTypeController::LoadModels(
 
   DataTypeActivationRequest request;
   request.error_handler = base::BindRepeating(
-      &ReportError, type(), base::SequencedTaskRunnerHandle::Get(),
+      &ReportError, base::SequencedTaskRunnerHandle::Get(),
       base::BindRepeating(&ModelTypeController::ReportModelError,
                           base::AsWeakPtr(this), SyncError::DATATYPE_ERROR));
   request.authenticated_account_id = configure_context.authenticated_account_id;
@@ -292,6 +287,11 @@ void ModelTypeController::ReportModelError(SyncError::ErrorType error_type,
                                            const ModelError& error) {
   DCHECK(CalledOnValidThread());
 
+  // TODO(wychen): enum uma should be strongly typed. crbug.com/661401
+  UMA_HISTOGRAM_ENUMERATION("Sync.DataTypeRunFailures",
+                            ModelTypeToHistogramInt(type()),
+                            static_cast<int>(MODEL_TYPE_COUNT));
+
   // Error could arrive too late, e.g. after the datatype has been stopped.
   // This is allowed for the delegate's convenience, so there's no constraints
   // around when exactly DataTypeActivationRequest::error_handler is supposed to

components/sync/driver/sync_driver_switches.cc

@@ -58,6 +58,8 @@ const base::Feature kSyncPseudoUSSExtensions{"SyncPseudoUSSExtensions",
                                              base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSFavicons{"SyncPseudoUSSFavicons",
                                            base::FEATURE_DISABLED_BY_DEFAULT};
+const base::Feature kSyncPseudoUSSHistoryDeleteDirectives{
+    "SyncPseudoUSSHistoryDeleteDirectives", base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSPreferences{
     "SyncPseudoUSSPreferences", base::FEATURE_DISABLED_BY_DEFAULT};
 const base::Feature kSyncPseudoUSSPriorityPreferences{

components/sync/driver/sync_driver_switches.h

@@ -28,6 +28,7 @@ extern const base::Feature kSyncPseudoUSSApps;
 extern const base::Feature kSyncPseudoUSSDictionary;
 extern const base::Feature kSyncPseudoUSSExtensions;
 extern const base::Feature kSyncPseudoUSSFavicons;
+extern const base::Feature kSyncPseudoUSSHistoryDeleteDirectives;
 extern const base::Feature kSyncPseudoUSSPreferences;
 extern const base::Feature kSyncPseudoUSSPriorityPreferences;
 extern const base::Feature kSyncPseudoUSSThemes;