[SyncFS] Make routines using metadata database run asynchronously.

Some methods not used in product code keep to call GetMetadataDatabase directly.


BUG=347425
TEST=./unit_tests --gtest_filter="DriveBackendSyncTest.*:SyncEngine*:Conflict*:RegisterApp*:RemoteTo*:LocalTo*:ListChange*:MetadataDatabase*:TaskDep*"

Review URL: https://codereview.chromium.org/306813002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274213 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync_file_system/drive_backend/drive_backend_sync_unittest.cc

@@ -16,6 +16,7 @@
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.h"
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.pb.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_engine.h"
+#include "chrome/browser/sync_file_system/drive_backend/sync_worker.h"
 #include "chrome/browser/sync_file_system/local/canned_syncable_file_system.h"
 #include "chrome/browser/sync_file_system/local/local_file_sync_context.h"
 #include "chrome/browser/sync_file_system/local/local_file_sync_service.h"
@@ -41,10 +42,11 @@ typedef fileapi::FileSystemOperation::FileEntryList FileEntryList;
 
 namespace {
 
-void SetSyncStatus(const base::Closure& closure,
-                   SyncStatusCode* status_out,
-                   SyncStatusCode status) {
-  *status_out = status;
+template <typename T>
+void SetValueAndCallClosure(const base::Closure& closure,
+                            T* arg_out,
+                            T arg) {
+  *arg_out = base::internal::CallbackForward(arg);
   closure.Run();
 }
 
@@ -158,8 +160,21 @@ class DriveBackendSyncTest : public testing::Test,
 
   bool GetAppRootFolderID(const std::string& app_id,
                           std::string* folder_id) {
+    base::RunLoop run_loop;
+    bool success = false;
     FileTracker tracker;
-    if (!metadata_database()->FindAppRootTracker(app_id, &tracker))
+    PostTaskAndReplyWithResult(
+        worker_task_runner_,
+        FROM_HERE,
+        base::Bind(&MetadataDatabase::FindAppRootTracker,
+                   base::Unretained(metadata_database()),
+                   app_id,
+                   &tracker),
+        base::Bind(&SetValueAndCallClosure<bool>,
+                   run_loop.QuitClosure(),
+                   &success));
+    run_loop.Run();
+    if (!success)
       return false;
     *folder_id = tracker.file_id();
     return true;
@@ -172,11 +187,25 @@ class DriveBackendSyncTest : public testing::Test,
 
   std::string GetFileIDByPath(const std::string& app_id,
                               const base::FilePath& path) {
+    base::RunLoop run_loop;
+    bool success = false;
     FileTracker tracker;
     base::FilePath result_path;
     base::FilePath normalized_path = path.NormalizePathSeparators();
-    EXPECT_TRUE(metadata_database()->FindNearestActiveAncestor(
-        app_id, normalized_path, &tracker, &result_path));
+    PostTaskAndReplyWithResult(
+        worker_task_runner_,
+        FROM_HERE,
+        base::Bind(&MetadataDatabase::FindNearestActiveAncestor,
+                   base::Unretained(metadata_database()),
+                   app_id,
+                   normalized_path,
+                   &tracker,
+                   &result_path),
+        base::Bind(&SetValueAndCallClosure<bool>,
+                   run_loop.QuitClosure(),
+                   &success));
+    run_loop.Run();
+    EXPECT_TRUE(success);
     EXPECT_EQ(normalized_path, result_path);
     return tracker.file_id();
   }
@@ -193,7 +222,8 @@ class DriveBackendSyncTest : public testing::Test,
       base::RunLoop run_loop;
       local_sync_service_->MaybeInitializeFileSystemContext(
           origin, file_system->file_system_context(),
-          base::Bind(&SetSyncStatus, run_loop.QuitClosure(), &status));
+          base::Bind(&SetValueAndCallClosure<SyncStatusCode>,
+                     run_loop.QuitClosure(), &status));
       run_loop.Run();
       EXPECT_EQ(SYNC_STATUS_OK, status);
 
@@ -208,7 +238,8 @@ class DriveBackendSyncTest : public testing::Test,
     base::RunLoop run_loop;
     remote_sync_service_->RegisterOrigin(
         origin,
-        base::Bind(&SetSyncStatus, run_loop.QuitClosure(), &status));
+        base::Bind(&SetValueAndCallClosure<SyncStatusCode>,
+                   run_loop.QuitClosure(), &status));
     run_loop.Run();
     return status;
   }
@@ -310,8 +341,17 @@ class DriveBackendSyncTest : public testing::Test,
         if (pending_remote_changes_ || pending_local_changes_)
           continue;
 
-        int64 largest_fetched_change_id =
-            metadata_database()->GetLargestFetchedChangeID();
+        base::RunLoop run_loop;
+        int64 largest_fetched_change_id = -1;
+        PostTaskAndReplyWithResult(
+            worker_task_runner_,
+            FROM_HERE,
+            base::Bind(&MetadataDatabase::GetLargestFetchedChangeID,
+                       base::Unretained(metadata_database())),
+            base::Bind(&SetValueAndCallClosure<int64>,
+                       run_loop.QuitClosure(),
+                       &largest_fetched_change_id));
+        run_loop.Run();
         if (largest_fetched_change_id != GetLargestChangeID()) {
           FetchRemoteChanges();
           continue;
@@ -487,11 +527,32 @@ class DriveBackendSyncTest : public testing::Test,
   }
 
   size_t CountMetadata() {
-    return metadata_database()->CountFileMetadata();
+    size_t count = 0;
+    base::RunLoop run_loop;
+    PostTaskAndReplyWithResult(
+        worker_task_runner_,
+        FROM_HERE,
+        base::Bind(&MetadataDatabase::CountFileMetadata,
+                   base::Unretained(metadata_database())),
+        base::Bind(&SetValueAndCallClosure<size_t>,
+                   run_loop.QuitClosure(),
+                   &count));
+    run_loop.Run();
+    return count;
   }
 
   size_t CountTracker() {
-    return metadata_database()->CountFileTracker();
+    size_t count = 0;
+    base::RunLoop run_loop;
+    PostTaskAndReplyWithResult(
+        worker_task_runner_,
+        FROM_HERE,
+        base::Bind(&MetadataDatabase::CountFileTracker,
+                   base::Unretained(metadata_database())),
+        base::Bind(&SetValueAndCallClosure<size_t>,
+                   run_loop.QuitClosure(), &count));
+    run_loop.Run();
+    return count;
   }
 
   drive::FakeDriveService* fake_drive_service() {
@@ -503,11 +564,13 @@ class DriveBackendSyncTest : public testing::Test,
     return fake_drive_service_helper_.get();
   }
 
+ private:
+  // NOTE: Member functions of MetadataDatabase class must not be called
+  // directly through this method.  Call them via PostTask.
   MetadataDatabase* metadata_database() {
-    return remote_sync_service_->GetMetadataDatabase();
+    return remote_sync_service_->sync_worker_->GetMetadataDatabase();
   }
 
- private:
   content::TestBrowserThreadBundle thread_bundle_;
 
   base::ScopedTempDir base_dir_;

chrome/browser/sync_file_system/drive_backend/sync_engine.cc

@@ -394,14 +394,10 @@ void SyncEngine::SetSyncEnabled(bool enabled) {
 }
 
 void SyncEngine::PromoteDemotedChanges() {
-  MetadataDatabase* metadata_db = GetMetadataDatabase();
-  if (metadata_db && metadata_db->HasLowPriorityDirtyTracker()) {
-    metadata_db->PromoteLowerPriorityTrackersToNormal();
-    FOR_EACH_OBSERVER(
-        Observer,
-        service_observers_,
-        OnRemoteChangeQueueUpdated(metadata_db->CountDirtyTracker()));
-  }
+  worker_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SyncWorker::PromoteDemotedChanges,
+                 base::Unretained(sync_worker_.get())));
 }
 
 void SyncEngine::ApplyLocalChange(
@@ -422,10 +418,6 @@ void SyncEngine::ApplyLocalChange(
                      FROM_HERE, callback)));
 }
 
-SyncTaskManager* SyncEngine::GetSyncTaskManagerForTesting() {
-  return sync_worker_->GetSyncTaskManager();
-}
-
 void SyncEngine::OnNotificationReceived() {
   worker_task_runner_->PostTask(
       FROM_HERE,
@@ -470,11 +462,6 @@ drive::DriveUploaderInterface* SyncEngine::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,
@@ -519,11 +506,11 @@ void SyncEngine::UpdateServiceState(RemoteServiceState state,
       OnRemoteServiceStateUpdated(state, description));
 }
 
-void SyncEngine::UpdateRegisteredApps() {
+void SyncEngine::UpdateRegisteredAppsForTesting() {
   if (!extension_service_)
     return;
 
-  MetadataDatabase* metadata_db = GetMetadataDatabase();
+  MetadataDatabase* metadata_db = sync_worker_->GetMetadataDatabase();
   DCHECK(metadata_db);
   std::vector<std::string> app_ids;
   metadata_db->GetRegisteredAppIDs(&app_ids);

chrome/browser/sync_file_system/drive_backend/sync_engine.h

@@ -121,8 +121,6 @@ class SyncEngine : public RemoteFileSyncService,
 
   drive::DriveServiceInterface* GetDriveService();
   drive::DriveUploaderInterface* GetDriveUploader();
-  MetadataDatabase* GetMetadataDatabase();
-  SyncTaskManager* GetSyncTaskManagerForTesting();
 
   void OnPendingFileListUpdated(int item_count);
   void OnFileStatusChanged(const fileapi::FileSystemURL& url,
@@ -145,7 +143,10 @@ class SyncEngine : public RemoteFileSyncService,
              ExtensionServiceInterface* extension_service,
              SigninManagerBase* signin_manager);
 
-  void UpdateRegisteredApps();
+  // TODO(peria): Migrate this method into test code.
+  // This method is not thread safe, because it requires to access metadata
+  // database which may live in another thread.
+  void UpdateRegisteredAppsForTesting();
 
   scoped_ptr<drive::DriveServiceInterface> drive_service_;
   scoped_ptr<DriveServiceWrapper> drive_service_wrapper_;

chrome/browser/sync_file_system/drive_backend/sync_engine_unittest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/sync_file_system/drive_backend/metadata_database.pb.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_task.h"
 #include "chrome/browser/sync_file_system/drive_backend/sync_task_manager.h"
+#include "chrome/browser/sync_file_system/drive_backend/sync_worker.h"
 #include "chrome/browser/sync_file_system/sync_file_system_test_util.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/common/extension.h"
@@ -136,11 +137,11 @@ class SyncEngineTest
   SyncEngine* sync_engine() { return sync_engine_.get(); }
 
   void UpdateRegisteredApps() {
-    sync_engine_->UpdateRegisteredApps();
+    sync_engine_->UpdateRegisteredAppsForTesting();
   }
 
   SyncTaskManager* GetSyncEngineTaskManager() {
-    return sync_engine_->GetSyncTaskManagerForTesting();
+    return sync_engine_->sync_worker_->GetSyncTaskManager();
   }
 
   void CheckServiceState(SyncStatusCode expected_sync_status,
@@ -150,6 +151,10 @@ class SyncEngineTest
     EXPECT_EQ(expected_service_status, sync_engine_->GetCurrentState());
   }
 
+  MetadataDatabase* metadata_database() {
+    return sync_engine()->sync_worker_->GetMetadataDatabase();
+  }
+
  private:
   content::TestBrowserThreadBundle browser_threads_;
   base::ScopedTempDir profile_dir_;
@@ -164,25 +169,24 @@ class SyncEngineTest
 TEST_F(SyncEngineTest, EnableOrigin) {
   FileTracker tracker;
   SyncStatusCode sync_status = SYNC_STATUS_UNKNOWN;
-  MetadataDatabase* metadata_database = sync_engine()->GetMetadataDatabase();
   GURL origin = extensions::Extension::GetBaseURLFromExtensionId(kAppID);
 
   sync_engine()->RegisterOrigin(origin, CreateResultReceiver(&sync_status));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(SYNC_STATUS_OK, sync_status);
-  ASSERT_TRUE(metadata_database->FindAppRootTracker(kAppID, &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker(kAppID, &tracker));
   EXPECT_EQ(TRACKER_KIND_APP_ROOT, tracker.tracker_kind());
 
   sync_engine()->DisableOrigin(origin, CreateResultReceiver(&sync_status));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(SYNC_STATUS_OK, sync_status);
-  ASSERT_TRUE(metadata_database->FindAppRootTracker(kAppID, &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker(kAppID, &tracker));
   EXPECT_EQ(TRACKER_KIND_DISABLED_APP_ROOT, tracker.tracker_kind());
 
   sync_engine()->EnableOrigin(origin, CreateResultReceiver(&sync_status));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(SYNC_STATUS_OK, sync_status);
-  ASSERT_TRUE(metadata_database->FindAppRootTracker(kAppID, &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker(kAppID, &tracker));
   EXPECT_EQ(TRACKER_KIND_APP_ROOT, tracker.tracker_kind());
 
   sync_engine()->UninstallOrigin(
@@ -191,7 +195,7 @@ TEST_F(SyncEngineTest, EnableOrigin) {
       CreateResultReceiver(&sync_status));
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(SYNC_STATUS_OK, sync_status);
-  ASSERT_FALSE(metadata_database->FindAppRootTracker(kAppID, &tracker));
+  ASSERT_FALSE(metadata_database()->FindAppRootTracker(kAppID, &tracker));
 }
 
 TEST_F(SyncEngineTest, UpdateRegisteredApps) {
@@ -214,16 +218,15 @@ TEST_F(SyncEngineTest, UpdateRegisteredApps) {
     EXPECT_EQ(SYNC_STATUS_OK, sync_status);
   }
 
-  MetadataDatabase* metadata_database = sync_engine()->GetMetadataDatabase();
   FileTracker tracker;
 
-  ASSERT_TRUE(metadata_database->FindAppRootTracker("app_0", &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker("app_0", &tracker));
   EXPECT_EQ(TRACKER_KIND_APP_ROOT, tracker.tracker_kind());
 
-  ASSERT_TRUE(metadata_database->FindAppRootTracker("app_1", &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker("app_1", &tracker));
   EXPECT_EQ(TRACKER_KIND_APP_ROOT, tracker.tracker_kind());
 
-  ASSERT_TRUE(metadata_database->FindAppRootTracker("app_2", &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker("app_2", &tracker));
   EXPECT_EQ(TRACKER_KIND_APP_ROOT, tracker.tracker_kind());
 
   extension_service()->DisableExtension("app_1");
@@ -232,13 +235,13 @@ TEST_F(SyncEngineTest, UpdateRegisteredApps) {
   UpdateRegisteredApps();
   base::RunLoop().RunUntilIdle();
 
-  ASSERT_TRUE(metadata_database->FindAppRootTracker("app_0", &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker("app_0", &tracker));
   EXPECT_EQ(TRACKER_KIND_APP_ROOT, tracker.tracker_kind());
 
-  ASSERT_TRUE(metadata_database->FindAppRootTracker("app_1", &tracker));
+  ASSERT_TRUE(metadata_database()->FindAppRootTracker("app_1", &tracker));
   EXPECT_EQ(TRACKER_KIND_DISABLED_APP_ROOT, tracker.tracker_kind());
 
-  ASSERT_FALSE(metadata_database->FindAppRootTracker("app_2", &tracker));
+  ASSERT_FALSE(metadata_database()->FindAppRootTracker("app_2", &tracker));
 }
 
 TEST_F(SyncEngineTest, GetOriginStatusMap) {

chrome/browser/sync_file_system/drive_backend/sync_worker.cc

@@ -245,12 +245,24 @@ void SyncWorker::SetSyncEnabled(bool enabled) {
     return;
 
   FOR_EACH_OBSERVER(
-      Observer, observers_,
+      Observer,
+      observers_,
       UpdateServiceState(
           GetCurrentState(),
           enabled ? "Sync is enabled" : "Sync is disabled"));
 }
 
+void SyncWorker::PromoteDemotedChanges() {
+  MetadataDatabase* metadata_db = GetMetadataDatabase();
+  if (metadata_db && metadata_db->HasLowPriorityDirtyTracker()) {
+    metadata_db->PromoteLowerPriorityTrackersToNormal();
+    FOR_EACH_OBSERVER(
+        Observer,
+        observers_,
+        OnPendingFileListUpdated(metadata_db->CountDirtyTracker()));
+  }
+}
+
 SyncStatusCode SyncWorker::SetDefaultConflictResolutionPolicy(
     ConflictResolutionPolicy policy) {
   default_conflict_resolution_policy_ = policy;

chrome/browser/sync_file_system/drive_backend/sync_worker.h

@@ -105,6 +105,7 @@ class SyncWorker : public SyncTaskManager::Client {
   scoped_ptr<base::ListValue> DumpFiles(const GURL& origin);
   scoped_ptr<base::ListValue> DumpDatabase();
   void SetSyncEnabled(bool enabled);
+  void PromoteDemotedChanges();
   SyncStatusCode SetDefaultConflictResolutionPolicy(
       ConflictResolutionPolicy policy);
   SyncStatusCode SetConflictResolutionPolicy(
@@ -137,9 +138,6 @@ class SyncWorker : public SyncTaskManager::Client {
   void AddObserver(Observer* observer);
 
  private:
-  friend class DriveBackendSyncTest;
-  friend class SyncEngineTest;
-
   void DoDisableApp(const std::string& app_id,
                     const SyncStatusCallback& callback);
   void DoEnableApp(const std::string& app_id,