[SyncFS] Post tasks between SyncEngine and SyncWorker

chrome/browser/sync_file_system/drive_backend/sync_engine.cc

Index: chrome/browser/sync_file_system/drive_backend/sync_engine.cc
diff --git a/chrome/browser/sync_file_system/drive_backend/sync_engine.cc b/chrome/browser/sync_file_system/drive_backend/sync_engine.cc
index bd296c15931e9867b9a180fedf0cac08ed14cd70..f959de6abda7f2d3cd14ee9ea7e6c62ec27913dc 100644
--- a/chrome/browser/sync_file_system/drive_backend/sync_engine.cc
+++ b/chrome/browser/sync_file_system/drive_backend/sync_engine.cc
@@ -98,11 +98,11 @@ scoped_ptr<SyncEngine> SyncEngine::CreateForBrowserContext(
   scoped_ptr<drive_backend::SyncEngine> sync_engine(
       new SyncEngine(drive_service.Pass(),
                      drive_uploader.Pass(),
+                     task_runner.get(),
                      notification_manager,
                      extension_service,
                      signin_manager));
   sync_engine->Initialize(GetSyncFileSystemDir(context->GetPath()),
-                          task_runner.get(),
                           NULL);
 
   return sync_engine.Pass();
@@ -125,12 +125,12 @@ SyncEngine::~SyncEngine() {
 }
 
 void SyncEngine::Initialize(const base::FilePath& base_dir,
-                            base::SequencedTaskRunner* task_runner,
                             leveldb::Env* env_override) {
   scoped_ptr<SyncEngineContext> sync_engine_context(
       new SyncEngineContext(drive_service_.get(),
                             drive_uploader_.get(),
-                            task_runner));
+                            worker_task_runner_.get(),

[tzik, 2014/04/17 17:48:32]

s/worker/file/?

Could you swap the order of these two task_runner?
I recently added a task runner to MetadataDatabase, and it's order was worker,
file.
I think maintaining the order in ui, worker, file is consistent to
MetadataDatabase case.

[peria, 2014/04/18 05:19:36]

Done.

+                            base::MessageLoopProxy::current()));
   // TODO(peria): Move this create function to thread pool.
   sync_worker_ = SyncWorker::CreateOnWorker(weak_ptr_factory_.GetWeakPtr(),
                                             base_dir,
@@ -153,33 +153,56 @@ void SyncEngine::AddFileStatusObserver(FileStatusObserver* observer) {
 
 void SyncEngine::RegisterOrigin(
     const GURL& origin, const SyncStatusCallback& callback) {
-  sync_worker_->RegisterOrigin(origin, callback);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::RegisterOrigin,
+                 base::Unretained(sync_worker_.get()),
+                 origin, callback));
 }
 
 void SyncEngine::EnableOrigin(
     const GURL& origin, const SyncStatusCallback& callback) {
-  sync_worker_->EnableOrigin(origin, callback);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::EnableOrigin,
+                 base::Unretained(sync_worker_.get()),
+                 origin, callback));
 }
 
 void SyncEngine::DisableOrigin(
     const GURL& origin, const SyncStatusCallback& callback) {
-  sync_worker_->DisableOrigin(origin, callback);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::DisableOrigin,
+                 base::Unretained(sync_worker_.get()),
+                 origin, callback));
 }
 
 void SyncEngine::UninstallOrigin(
     const GURL& origin,
     UninstallFlag flag,
     const SyncStatusCallback& callback) {
-  sync_worker_->UninstallOrigin(origin, flag, callback);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::UninstallOrigin,
+                 base::Unretained(sync_worker_.get()),
+                 origin, flag, callback));
 }
 
 void SyncEngine::ProcessRemoteChange(const SyncFileCallback& callback) {
-  sync_worker_->ProcessRemoteChange(callback);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::ProcessRemoteChange,
+                 base::Unretained(sync_worker_.get()),
+                 callback));
 }
 
-void SyncEngine::SetRemoteChangeProcessor(
-    RemoteChangeProcessor* processor) {
-  sync_worker_->SetRemoteChangeProcessor(processor);
+void SyncEngine::SetRemoteChangeProcessor(RemoteChangeProcessor* processor) {
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::SetRemoteChangeProcessor,
+                 base::Unretained(sync_worker_.get()),
+                 processor));
 }
 
 LocalChangeProcessor* SyncEngine::GetLocalChangeProcessor() {
@@ -192,23 +215,31 @@ bool SyncEngine::IsConflicting(const fileapi::FileSystemURL& url) {
 }
 
 RemoteServiceState SyncEngine::GetCurrentState() const {
+  // TODO(peria): Post task
   return sync_worker_->GetCurrentState();
 }
 
 void SyncEngine::GetOriginStatusMap(OriginStatusMap* status_map) {
+  // TODO(peria): Make this route asynchronous.
   sync_worker_->GetOriginStatusMap(status_map);
 }
 
 scoped_ptr<base::ListValue> SyncEngine::DumpFiles(const GURL& origin) {
+  // TODO(peria): Make this route asynchronous.
   return sync_worker_->DumpFiles(origin);
 }
 
 scoped_ptr<base::ListValue> SyncEngine::DumpDatabase() {
+  // TODO(peria): Make this route asynchronous.
   return sync_worker_->DumpDatabase();
 }
 
 void SyncEngine::SetSyncEnabled(bool enabled) {
-  sync_worker_->SetSyncEnabled(enabled);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::SetSyncEnabled,
+                 base::Unretained(sync_worker_.get()),
+                 enabled));
 }
 
 void SyncEngine::UpdateSyncEnabled(bool enabled) {
@@ -220,22 +251,26 @@ void SyncEngine::UpdateSyncEnabled(bool enabled) {
 
 SyncStatusCode SyncEngine::SetDefaultConflictResolutionPolicy(
     ConflictResolutionPolicy policy) {
+  // TODO(peria): Make this route asynchronous.
   return sync_worker_->SetDefaultConflictResolutionPolicy(policy);
 }
 
 SyncStatusCode SyncEngine::SetConflictResolutionPolicy(
     const GURL& origin,
     ConflictResolutionPolicy policy) {
+  // TODO(peria): Make this route asynchronous.
   return sync_worker_->SetConflictResolutionPolicy(origin, policy);
 }
 
 ConflictResolutionPolicy SyncEngine::GetDefaultConflictResolutionPolicy()
     const {
+  // TODO(peria): Make this route asynchronous.
   return sync_worker_->GetDefaultConflictResolutionPolicy();
 }
 
 ConflictResolutionPolicy SyncEngine::GetConflictResolutionPolicy(
     const GURL& origin) const {
+  // TODO(peria): Make this route asynchronous.
   return sync_worker_->GetConflictResolutionPolicy(origin);
 }
 
@@ -271,49 +306,75 @@ void SyncEngine::ApplyLocalChange(
     const SyncFileMetadata& local_metadata,
     const fileapi::FileSystemURL& url,
     const SyncStatusCallback& callback) {
-  sync_worker_->ApplyLocalChange(
-      local_change, local_path, local_metadata, url, callback);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::ApplyLocalChange,
+                 base::Unretained(sync_worker_.get()),
+                 local_change,
+                 local_path,
+                 local_metadata,
+                 url,
+                 callback));
 }
 
 SyncTaskManager* SyncEngine::GetSyncTaskManagerForTesting() {
+  // TODO(peria): Post task
   return sync_worker_->GetSyncTaskManager();
 }
 
 void SyncEngine::OnNotificationReceived() {
-  sync_worker_->OnNotificationReceived();
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::OnNotificationReceived,
+                 base::Unretained(sync_worker_.get())));
 }
 
 void SyncEngine::OnPushNotificationEnabled(bool) {}
 
 void SyncEngine::OnReadyToSendRequests() {
-  sync_worker_->OnReadyToSendRequests(
-      signin_manager_ ? signin_manager_->GetAuthenticatedAccountId() : "");
+  const std::string& account_id =

[tzik, 2014/04/17 17:48:32]

Can we use value instead of const ref here?
Even though this lifetime expansion is legal, I feel a bit uneasy.

[peria, 2014/04/18 05:19:36]

Done.

+      signin_manager_ ? signin_manager_->GetAuthenticatedAccountId() : "";
+
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::OnReadyToSendRequests,
+                 base::Unretained(sync_worker_.get()),
+                 account_id));
 }
 
 void SyncEngine::OnRefreshTokenInvalid() {
-  sync_worker_->OnRefreshTokenInvalid();
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::OnRefreshTokenInvalid,
+                 base::Unretained(sync_worker_.get())));
 }
 
 void SyncEngine::OnNetworkChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
-  sync_worker_->OnNetworkChanged(type);
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::OnNetworkChanged,
+                 base::Unretained(sync_worker_.get()),
+                 type));
 }
 
 drive::DriveServiceInterface* SyncEngine::GetDriveService() {
-  return sync_worker_->GetDriveService();
+  return drive_service_.get();
 }
 
 drive::DriveUploaderInterface* SyncEngine::GetDriveUploader() {
-  return sync_worker_->GetDriveUploader();
+  return drive_uploader_.get();
 }
 
 MetadataDatabase* SyncEngine::GetMetadataDatabase() {
+  // TODO(peria): Post task
   return sync_worker_->GetMetadataDatabase();
 }
 
 SyncEngine::SyncEngine(
     scoped_ptr<drive::DriveServiceInterface> drive_service,
     scoped_ptr<drive::DriveUploaderInterface> drive_uploader,
+    base::SequencedTaskRunner* worker_task_runner,

[tzik, 2014/04/17 17:48:32]

s/worker/file/?

[peria, 2014/04/18 05:19:36]

Task runner for file I/O is passed in Initialization().

     drive::DriveNotificationManager* notification_manager,
     ExtensionServiceInterface* extension_service,
     SigninManagerBase* signin_manager)
@@ -322,6 +383,7 @@ SyncEngine::SyncEngine(
       notification_manager_(notification_manager),
       extension_service_(extension_service),
       signin_manager_(signin_manager),
+      worker_task_runner_(worker_task_runner),
       weak_ptr_factory_(this) {}
 
 void SyncEngine::DidProcessRemoteChange(RemoteToLocalSyncer* syncer) {