Move ProfileSyncService::background_task_runner_ to SyncEngineImpl

ProfileSyncService mostly creates the task runner and passes it to the
engine, so this CL moves the creation to the engine. The one use of
the runner by ProfileSyncService is replaced with posting a task to the
thread pool.

Bug: 1123881
Change-Id: Ifef2806b527424b30e2a0d6ef07cfca026ee07b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2461325Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#816106}

components/sync/driver/glue/sync_engine_impl.cc

@@ -11,6 +11,8 @@
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "base/logging.h"
+#include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 #include "base/task_runner_util.h"
 #include "build/build_config.h"
 #include "components/invalidation/impl/invalidation_switches.h"
@@ -42,7 +44,10 @@ SyncEngineImpl::SyncEngineImpl(
     SyncInvalidationsService* sync_invalidations_service,
     const base::WeakPtr<SyncPrefs>& sync_prefs,
     const base::FilePath& sync_data_folder)
-    : name_(name),
+    : sync_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
+          {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
+           base::TaskShutdownBehavior::BLOCK_SHUTDOWN})),
+      name_(name),
       sync_prefs_(sync_prefs),
       invalidator_(invalidator),
       sync_invalidations_service_(sync_invalidations_service),
@@ -61,11 +66,9 @@ SyncEngineImpl::~SyncEngineImpl() {
 }
 
 void SyncEngineImpl::Initialize(InitParams params) {
-  DCHECK(params.sync_task_runner);
   DCHECK(params.host);
   DCHECK(params.registrar);
 
-  sync_task_runner_ = params.sync_task_runner;
   host_ = params.host;
   registrar_ = params.registrar.get();
 

components/sync/driver/glue/sync_engine_impl.h

@@ -18,6 +18,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
 #include "components/invalidation/public/invalidation_handler.h"
 #include "components/sync/base/extensions_activity.h"
 #include "components/sync/base/model_type.h"

components/sync/driver/glue/sync_engine_impl_unittest.cc

@@ -21,7 +21,6 @@
 #include "base/test/task_environment.h"
 #include "base/test/test_timeouts.h"
 #include "base/threading/sequenced_task_runner_handle.h"
-#include "base/threading/thread.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "components/invalidation/impl/invalidation_logger.h"
@@ -183,8 +182,7 @@ std::unique_ptr<HttpPostProviderFactory> CreateHttpBridgeFactory() {
 class SyncEngineImplTest : public testing::Test {
  protected:
   SyncEngineImplTest()
-      : sync_thread_("SyncThreadForTest"),
-        host_(base::BindOnce(&SyncEngineImplTest::SetEngineTypes,
+      : host_(base::BindOnce(&SyncEngineImplTest::SetEngineTypes,
                              base::Unretained(this))),
         fake_manager_(nullptr) {}
 
@@ -196,7 +194,6 @@ class SyncEngineImplTest : public testing::Test {
     SyncPrefs::RegisterProfilePrefs(pref_service_.registry());
 
     sync_prefs_ = std::make_unique<SyncPrefs>(&pref_service_);
-    sync_thread_.StartAndWaitForTesting();
     ON_CALL(invalidator_, UpdateInterestedTopics(_, _))
         .WillByDefault(testing::Return(true));
     backend_ = std::make_unique<SyncEngineImpl>(
@@ -235,7 +232,6 @@ class SyncEngineImplTest : public testing::Test {
     host_.SetExpectSuccess(expect_success);
 
     SyncEngine::InitParams params;
-    params.sync_task_runner = sync_thread_.task_runner();
     params.host = &host_;
     params.registrar = std::make_unique<SyncBackendRegistrar>(
         std::string(), base::BindRepeating(&CreateModelWorkerForGroup));
@@ -311,7 +307,6 @@ class SyncEngineImplTest : public testing::Test {
   base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir temp_dir_;
   sync_preferences::TestingPrefServiceSyncable pref_service_;
-  base::Thread sync_thread_;
   TestSyncEngineHost host_;
   std::unique_ptr<SyncPrefs> sync_prefs_;
   std::unique_ptr<SyncEngineImpl> backend_;

components/sync/driver/profile_sync_service.cc

@@ -237,9 +237,6 @@ ProfileSyncService::ProfileSyncService(InitParams init_params)
                               base::Unretained(this)),
           &sync_prefs_,
           sync_client_->GetTrustedVaultClient()),
-      backend_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
-          {base::MayBlock(), base::TaskPriority::USER_VISIBLE,
-           base::TaskShutdownBehavior::BLOCK_SHUTDOWN})),
       network_time_update_callback_(
           std::move(init_params.network_time_update_callback)),
       url_loader_factory_(std::move(init_params.url_loader_factory)),
@@ -580,7 +577,6 @@ void ProfileSyncService::StartUpSlowEngineComponents() {
   }
 
   SyncEngine::InitParams params;
-  params.sync_task_runner = backend_task_runner_;
   params.host = this;
   params.registrar = std::make_unique<SyncBackendRegistrar>(
       debug_identifier_,
@@ -663,8 +659,10 @@ void ProfileSyncService::ShutdownImpl(ShutdownReason reason) {
       // certain codepaths such as the user being signed out). To avoid that,
       // SyncPrefs is used to determine whether it's worth it.
       if (!sync_prefs_.GetCacheGuid().empty()) {
-        backend_task_runner_->PostTask(
+        base::ThreadPool::PostTask(
             FROM_HERE,
+            {base::TaskPriority::USER_VISIBLE, base::MayBlock(),
+             base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
             base::BindOnce(&DeleteLegacyDirectoryFilesAndNigoriStorage,
                            sync_client_->GetSyncDataPath()));
       }

components/sync/driver/profile_sync_service.h

@@ -411,9 +411,6 @@ class ProfileSyncService : public SyncService,
   // A utility object containing logic and state relating to encryption.
   SyncServiceCrypto crypto_;
 
-  // TODO(crbug.com/923287): Move out of this class. Possibly to SyncEngineImpl.
-  scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
-
   // Our asynchronous engine to communicate with sync components living on
   // other threads.
   std::unique_ptr<SyncEngine> engine_;

components/sync/engine/sync_engine.h

@@ -15,7 +15,6 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/sequenced_task_runner.h"
 #include "base/time/time.h"
 #include "components/sync/base/extensions_activity.h"
 #include "components/sync/base/model_type.h"
@@ -51,7 +50,6 @@ class SyncEngine : public ModelTypeConfigurer {
     InitParams(InitParams&& other);
     ~InitParams();
 
-    scoped_refptr<base::SequencedTaskRunner> sync_task_runner;
     SyncEngineHost* host = nullptr;
     std::unique_ptr<SyncBackendRegistrar> registrar;
     std::unique_ptr<SyncEncryptionHandler::Observer> encryption_observer_proxy;