Add to be fetched files instead of DriveMetadataStore.batch_sync_origins_

This persists files to be fetched to the DB so they can be
restored in case Chrome crashes. The origin itself is no
longer saved but it does get added back to
pending_batch_sync_origins anyway when the SyncFS app is
loaded again.

Pre-requisite to deprecating batch_sync_origins from
DriveMetadataStore

BUG=229764
TEST=Existing unit_tests --gtest_filter=DriveFileSyncService*

Review URL: https://chromiumcodereview.appspot.com/15023022

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

chrome/browser/sync_file_system/drive_file_sync_service.h

@@ -242,6 +242,7 @@ class DriveFileSyncService : public RemoteFileSyncService,
   void DidGetDirectoryContentForBatchSync(
       const SyncStatusCallback& callback,
       const GURL& origin,
+      const std::string& resource_id,
       int64 largest_changestamp,
       google_apis::GDataErrorCode error,
       scoped_ptr<google_apis::ResourceList> feed);
@@ -400,7 +401,7 @@ class DriveFileSyncService : public RemoteFileSyncService,
 
   int64 largest_fetched_changestamp_;
 
-  std::set<GURL> pending_batch_sync_origins_;
+  std::map<GURL, std::string> pending_batch_sync_origins_;
 
   // Is set to true when there's a fair possibility that we have some
   // remote changes that haven't been fetched yet.

chrome/browser/sync_file_system/drive_file_sync_service_sync_unittest.cc

@@ -173,8 +173,8 @@ class DriveFileSyncServiceSyncTest : public testing::Test {
     EXPECT_TRUE(done);
 
     metadata_store_->SetSyncRootDirectory(kSyncRootResourceId);
-    metadata_store_->AddBatchSyncOrigin(GURL(kAppOrigin), kParentResourceId);
-    metadata_store_->MoveBatchSyncOriginToIncremental(GURL(kAppOrigin));
+    metadata_store_->AddIncrementalSyncOrigin(GURL(kAppOrigin),
+                                              kParentResourceId);
 
     sync_service_ = DriveFileSyncService::CreateForTesting(
         &profile_,

chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc

@@ -115,8 +115,9 @@ TEST_F(DriveFileSyncServiceTest, DeleteOriginDirectory) {
 
   // Add meta_data entry so GURL->resourse_id mapping is there.
   const GURL origin_gurl("chrome-extension://uninstallme");
-  metadata_store()->AddBatchSyncOrigin(origin_gurl,
-                                       origin_dir_resource_id);
+  metadata_store()->AddIncrementalSyncOrigin(origin_gurl,
+                                             origin_dir_resource_id);
+
   // Delete the origin directory.
   bool done = false;
   sync_service()->UninstallOrigin(

chrome/browser/sync_file_system/drive_metadata_store.cc

@@ -471,6 +471,13 @@ SyncStatusCode DriveMetadataStore::ReadEntry(const FileSystemURL& url,
   return SYNC_STATUS_OK;
 }
 
+void DriveMetadataStore::AddIncrementalSyncOrigin(
+    const GURL& origin,
+    const std::string& resource_id) {
+  AddBatchSyncOrigin(origin, resource_id);
+  MoveBatchSyncOriginToIncremental(origin);
+}
+
 void DriveMetadataStore::SetSyncRootDirectory(const std::string& resource_id) {
   DCHECK(CalledOnValidThread());
 
@@ -533,46 +540,6 @@ bool DriveMetadataStore::IsOriginDisabled(const GURL& origin) const {
   return ContainsKey(disabled_origins_, origin);
 }
 
-void DriveMetadataStore::AddBatchSyncOrigin(const GURL& origin,
-                                            const std::string& resource_id) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!IsBatchSyncOrigin(origin));
-  DCHECK(!IsIncrementalSyncOrigin(origin));
-  DCHECK(!IsOriginDisabled(origin));
-  DCHECK_EQ(SYNC_STATUS_OK, db_status_);
-
-  batch_sync_origins_.insert(std::make_pair(origin, resource_id));
-  origin_by_resource_id_.insert(std::make_pair(resource_id, origin));
-
-  // Store a pair of |origin| and |resource_id| in the DB.
-  base::PostTaskAndReplyWithResult(
-      file_task_runner_, FROM_HERE,
-      base::Bind(&DriveMetadataDB::UpdateOriginAsBatchSync,
-                 base::Unretained(db_.get()), origin, resource_id),
-      base::Bind(&DriveMetadataStore::UpdateDBStatus, AsWeakPtr()));
-}
-
-void DriveMetadataStore::MoveBatchSyncOriginToIncremental(const GURL& origin) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(IsBatchSyncOrigin(origin));
-  DCHECK(!IsIncrementalSyncOrigin(origin));
-  DCHECK(!IsOriginDisabled(origin));
-  DCHECK_EQ(SYNC_STATUS_OK, db_status_);
-
-  std::map<GURL, std::string>::iterator found =
-      batch_sync_origins_.find(origin);
-  incremental_sync_origins_.insert(std::make_pair(origin, found->second));
-
-  // Store a pair of |origin| and |resource_id| in the DB.
-  base::PostTaskAndReplyWithResult(
-      file_task_runner_, FROM_HERE,
-      base::Bind(&DriveMetadataDB::UpdateOriginAsIncrementalSync,
-                 base::Unretained(db_.get()), origin, found->second),
-      base::Bind(&DriveMetadataStore::UpdateDBStatus, AsWeakPtr()));
-
-  batch_sync_origins_.erase(found);
-}
-
 void DriveMetadataStore::EnableOrigin(
     const GURL& origin,
     const SyncStatusCallback& callback) {
@@ -586,8 +553,8 @@ void DriveMetadataStore::EnableOrigin(
   std::string resource_id = found->second;
   disabled_origins_.erase(found);
 
-  // Ensure |origin| is marked as a batch sync origin.
-  batch_sync_origins_.insert(std::make_pair(origin, resource_id));
+  // |Origin| goes back to DriveFileSyncService.pending_batch_sync_origins_
+  // only and is not stored in drive_metadata_store.
   found = incremental_sync_origins_.find(origin);
   if (found != incremental_sync_origins_.end())
     incremental_sync_origins_.erase(found);
@@ -662,6 +629,48 @@ void DriveMetadataStore::DidUpdateOrigin(
   callback.Run(status);
 }
 
+void DriveMetadataStore::AddBatchSyncOrigin(const GURL& origin,
+                                            const std::string& resource_id) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(!IsIncrementalSyncOrigin(origin));
+  DCHECK(!IsOriginDisabled(origin));
+  DCHECK_EQ(SYNC_STATUS_OK, db_status_);
+
+  if (IsBatchSyncOrigin(origin))
+    return;
+
+  batch_sync_origins_.insert(std::make_pair(origin, resource_id));
+  origin_by_resource_id_.insert(std::make_pair(resource_id, origin));
+
+  // Store a pair of |origin| and |resource_id| in the DB.
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_, FROM_HERE,
+      base::Bind(&DriveMetadataDB::UpdateOriginAsBatchSync,
+                 base::Unretained(db_.get()), origin, resource_id),
+      base::Bind(&DriveMetadataStore::UpdateDBStatus, AsWeakPtr()));
+}
+
+void DriveMetadataStore::MoveBatchSyncOriginToIncremental(const GURL& origin) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsBatchSyncOrigin(origin));
+  DCHECK(!IsIncrementalSyncOrigin(origin));
+  DCHECK(!IsOriginDisabled(origin));
+  DCHECK_EQ(SYNC_STATUS_OK, db_status_);
+
+  std::map<GURL, std::string>::iterator found =
+      batch_sync_origins_.find(origin);
+  incremental_sync_origins_.insert(std::make_pair(origin, found->second));
+
+  // Store a pair of |origin| and |resource_id| in the DB.
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_, FROM_HERE,
+      base::Bind(&DriveMetadataDB::UpdateOriginAsIncrementalSync,
+                 base::Unretained(db_.get()), origin, found->second),
+      base::Bind(&DriveMetadataStore::UpdateDBStatus, AsWeakPtr()));
+
+  batch_sync_origins_.erase(found);
+}
+
 void DriveMetadataStore::UpdateDBStatus(SyncStatusCode status) {
   DCHECK(CalledOnValidThread());
   if (db_status_ != SYNC_STATUS_OK &&

chrome/browser/sync_file_system/drive_metadata_store.h

@@ -73,6 +73,12 @@ class DriveMetadataStore
   SyncStatusCode ReadEntry(const fileapi::FileSystemURL& url,
                            DriveMetadata* metadata) const;
 
+  // Marks |origin| as incremental sync and associates it with the directory
+  // identified by |resource_id|.
+  // |origin| must not already be an incremental sync origin.
+  void AddIncrementalSyncOrigin(const GURL& origin,
+                                const std::string& resource_id);
+
   // Returns true if |origin| is a batch sync origin, a incremental sync origin
   // or a disabled origin.
   bool IsKnownOrigin(const GURL& origin) const;
@@ -89,15 +95,6 @@ class DriveMetadataStore
   // Returns true if |origin| is a disabled origin.
   bool IsOriginDisabled(const GURL& origin) const;
 
-  // Marks |origin| as a batch sync origin and associates it with the directory
-  // identified by |resource_id|.
-  // |origin| must not be a batch sync origin nor an incremental sync origin.
-  void AddBatchSyncOrigin(const GURL& origin, const std::string& resource_id);
-
-  // Marks |origin| as an incremental sync origin.
-  // |origin| must be a batch sync origin.
-  void MoveBatchSyncOriginToIncremental(const GURL& origin);
-
   void EnableOrigin(const GURL& origin,
                     const SyncStatusCallback& callback);
 
@@ -158,6 +155,15 @@ class DriveMetadataStore
  private:
   friend class DriveMetadataStoreTest;
 
+  // Marks |origin| as a batch sync origin and associates it with the directory
+  // identified by |resource_id|.
+  // |origin| must not be a batch sync origin nor an incremental sync origin.
+  void AddBatchSyncOrigin(const GURL& origin, const std::string& resource_id);
+
+  // Marks |origin| as an incremental sync origin.
+  // |origin| must be a batch sync origin.
+  void MoveBatchSyncOriginToIncremental(const GURL& origin);
+
   void UpdateDBStatus(SyncStatusCode status);
   void UpdateDBStatusAndInvokeCallback(const SyncStatusCallback& callback,
                                        SyncStatusCode status);

chrome/browser/sync_file_system/drive_metadata_store_unittest.cc

@@ -221,15 +221,6 @@ class DriveMetadataStoreTest : public testing::Test {
     EXPECT_FALSE(metadata_store()->IsOriginDisabled(origin));
   }
 
-  void VerifyBatchSyncOrigin(const GURL& origin,
-                             const std::string& resource_id) {
-    EXPECT_TRUE(metadata_store()->IsBatchSyncOrigin(origin));
-    EXPECT_FALSE(metadata_store()->IsIncrementalSyncOrigin(origin));
-    EXPECT_FALSE(metadata_store()->IsOriginDisabled(origin));
-    EXPECT_EQ(resource_id,
-              GetResourceID(metadata_store()->batch_sync_origins(), origin));
-  }
-
   void VerifyIncrementalSyncOrigin(const GURL& origin,
                                    const std::string& resource_id) {
     EXPECT_FALSE(metadata_store()->IsBatchSyncOrigin(origin));
@@ -448,97 +439,77 @@ TEST_F(DriveMetadataStoreTest, StoreSyncRootDirectory) {
 TEST_F(DriveMetadataStoreTest, StoreSyncOrigin) {
   const GURL kOrigin1("chrome-extension://example1");
   const GURL kOrigin2("chrome-extension://example2");
-  const GURL kOrigin3("chrome-extension://example3");
   const std::string kResourceId1("hoge");
   const std::string kResourceId2("fuga");
-  const std::string kResourceId3("foo");
 
   InitializeDatabase();
 
   // Make sure origins have not been marked yet.
   VerifyUntrackedOrigin(kOrigin1);
   VerifyUntrackedOrigin(kOrigin2);
-  VerifyUntrackedOrigin(kOrigin3);
-
-  // Mark origins as batch sync origins.
-  metadata_store()->AddBatchSyncOrigin(kOrigin1, kResourceId1);
-  metadata_store()->AddBatchSyncOrigin(kOrigin2, kResourceId2);
-  metadata_store()->AddBatchSyncOrigin(kOrigin3, kResourceId3);
-  VerifyBatchSyncOrigin(kOrigin1, kResourceId1);
-  VerifyBatchSyncOrigin(kOrigin2, kResourceId2);
-  VerifyBatchSyncOrigin(kOrigin3, kResourceId3);
-
-  // Mark |kOrigin1| as an incremental sync origin, and disable |kOrigin3| as
-  // a disabled sync origin. |kOrigin2| should have still been marked as a
-  // batch sync origin.
-  metadata_store()->MoveBatchSyncOriginToIncremental(kOrigin1);
-  DisableOrigin(kOrigin3);
+
+  // Mark origins as incremental sync origins.
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin1, kResourceId1);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin2, kResourceId2);
   VerifyIncrementalSyncOrigin(kOrigin1, kResourceId1);
-  VerifyBatchSyncOrigin(kOrigin2, kResourceId2);
-  VerifyDisabledOrigin(kOrigin3, kResourceId3);
+  VerifyIncrementalSyncOrigin(kOrigin2, kResourceId2);
+
+  // Disabled origin 2, origin 1 should still be incremental.
+  DisableOrigin(kOrigin2);
+  VerifyIncrementalSyncOrigin(kOrigin1, kResourceId1);
+  VerifyDisabledOrigin(kOrigin2, kResourceId2);
 
   DropSyncOriginsInStore();
 
   // Make sure origins have been dropped.
   VerifyUntrackedOrigin(kOrigin1);
   VerifyUntrackedOrigin(kOrigin2);
-  VerifyUntrackedOrigin(kOrigin3);
 
   RestoreOriginsFromDB();
 
   // Make sure origins have been restored.
   VerifyIncrementalSyncOrigin(kOrigin1, kResourceId1);
-  VerifyBatchSyncOrigin(kOrigin2, kResourceId2);
-  VerifyDisabledOrigin(kOrigin3, kResourceId3);
+  VerifyDisabledOrigin(kOrigin2, kResourceId2);
 
   VerifyReverseMap();
 }
 
 TEST_F(DriveMetadataStoreTest, DisableOrigin) {
   const GURL kOrigin1("chrome-extension://example1");
-  const GURL kOrigin2("chrome-extension://example2");
   const std::string kResourceId1("hoge");
-  const std::string kResourceId2("fuga");
 
   InitializeDatabase();
   EXPECT_EQ(SYNC_STATUS_OK, SetLargestChangeStamp(1));
 
-  metadata_store()->AddBatchSyncOrigin(kOrigin1, kResourceId1);
-  metadata_store()->AddBatchSyncOrigin(kOrigin2, kResourceId2);
-  metadata_store()->MoveBatchSyncOriginToIncremental(kOrigin2);
-  VerifyBatchSyncOrigin(kOrigin1, kResourceId1);
-  VerifyIncrementalSyncOrigin(kOrigin2, kResourceId2);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin1, kResourceId1);
+  VerifyIncrementalSyncOrigin(kOrigin1, kResourceId1);
 
   DisableOrigin(kOrigin1);
-  DisableOrigin(kOrigin2);
   VerifyDisabledOrigin(kOrigin1, kResourceId1);
-  VerifyDisabledOrigin(kOrigin2, kResourceId2);
 
+  // Re-enabled origins go back to DriveFileSyncService and are not tracked
+  // in DriveMetadataStore.
   EnableOrigin(kOrigin1);
-  EnableOrigin(kOrigin2);
-  VerifyBatchSyncOrigin(kOrigin1, kResourceId1);
-  VerifyBatchSyncOrigin(kOrigin2, kResourceId2);
+  VerifyUntrackedOrigin(kOrigin1);
 }
 
 TEST_F(DriveMetadataStoreTest, RemoveOrigin) {
   const GURL kOrigin1("chrome-extension://example1");
   const GURL kOrigin2("chrome-extension://example2");
   const GURL kOrigin3("chrome-extension://example3");
-  const GURL kOrigin4("chrome-extension://example4");
-  const GURL kOrigin5("chrome-extension://example5");
   const std::string kResourceId1("hogera");
   const std::string kResourceId2("fugaga");
   const std::string kResourceId3("piyopiyo");
-  const std::string kResourceId5("hogehoge");
 
   InitializeDatabase();
   EXPECT_EQ(SYNC_STATUS_OK, SetLargestChangeStamp(1));
 
-  metadata_store()->AddBatchSyncOrigin(kOrigin1, kResourceId1);
-  metadata_store()->AddBatchSyncOrigin(kOrigin2, kResourceId2);
-  metadata_store()->MoveBatchSyncOriginToIncremental(kOrigin2);
-  metadata_store()->AddBatchSyncOrigin(kOrigin3, kResourceId3);
-  metadata_store()->AddBatchSyncOrigin(kOrigin5, kResourceId5);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin1, kResourceId1);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin2, kResourceId2);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin3, kResourceId3);
+  DisableOrigin(kOrigin3);
+  EXPECT_EQ(2u, metadata_store()->incremental_sync_origins().size());
+  EXPECT_EQ(1u, metadata_store()->disabled_origins().size());
 
   EXPECT_EQ(SYNC_STATUS_OK,
             UpdateEntry(
@@ -555,39 +526,22 @@ TEST_F(DriveMetadataStoreTest, RemoveOrigin) {
                 CreateSyncableFileSystemURL(
                     kOrigin3, kServiceName, base::FilePath(FPL("waf"))),
                 CreateMetadata("baz", "egg", false, false)));
-  EXPECT_EQ(SYNC_STATUS_OK,
-            UpdateEntry(
-                CreateSyncableFileSystemURL(
-                    kOrigin4, kServiceName, base::FilePath(FPL("cue"))),
-                CreateMetadata("lat", "fork", false, false)));
-  EXPECT_EQ(SYNC_STATUS_OK,
-            UpdateEntry(
-                CreateSyncableFileSystemURL(
-                    kOrigin5, kServiceName, base::FilePath(FPL("tac"))),
-                CreateMetadata("pov", "spoon", false, false)));
-  EXPECT_EQ(SYNC_STATUS_OK,
-            UpdateEntry(
-                CreateSyncableFileSystemURL(
-                    kOrigin1, kServiceName, base::FilePath(FPL("tic"))),
-                CreateMetadata("zav", "sause", false, false)));
 
-  EXPECT_EQ(SYNC_STATUS_OK, RemoveOrigin(kOrigin1));
   EXPECT_EQ(SYNC_STATUS_OK, RemoveOrigin(kOrigin2));
-  EXPECT_EQ(SYNC_STATUS_OK, RemoveOrigin(kOrigin4));
-  EXPECT_EQ(SYNC_STATUS_OK, RemoveOrigin(kOrigin5));
+  EXPECT_EQ(SYNC_STATUS_OK, RemoveOrigin(kOrigin3));
 
   DropDatabase();
   InitializeDatabase();
 
-  // kOrigin3 should be only remaining batch sync origin.
-  EXPECT_EQ(1u, metadata_store()->batch_sync_origins().size());
-  EXPECT_TRUE(metadata_store()->IsBatchSyncOrigin(kOrigin3));
-  EXPECT_TRUE(metadata_store()->incremental_sync_origins().empty());
-  EXPECT_TRUE(metadata_store()->disabled_origins().empty());
+  // kOrigin1 should be the only one left.
+  EXPECT_EQ(0u, metadata_store()->batch_sync_origins().size());
+  EXPECT_EQ(1u, metadata_store()->incremental_sync_origins().size());
+  EXPECT_EQ(0u, metadata_store()->disabled_origins().size());
+  EXPECT_TRUE(metadata_store()->IsIncrementalSyncOrigin(kOrigin1));
   EXPECT_EQ(1u, metadata_map().size());
 
   DriveMetadataStore::MetadataMap::const_iterator found =
-      metadata_map().find(kOrigin3);
+      metadata_map().find(kOrigin1);
   EXPECT_TRUE(found != metadata_map().end() && found->second.size() == 1u);
 
   VerifyReverseMap();
@@ -605,10 +559,9 @@ TEST_F(DriveMetadataStoreTest, GetResourceIdForOrigin) {
   EXPECT_EQ(SYNC_STATUS_OK, SetLargestChangeStamp(1));
   metadata_store()->SetSyncRootDirectory("root");
 
-  metadata_store()->AddBatchSyncOrigin(kOrigin1, kResourceId1);
-  metadata_store()->AddBatchSyncOrigin(kOrigin2, kResourceId2);
-  metadata_store()->MoveBatchSyncOriginToIncremental(kOrigin2);
-  metadata_store()->AddBatchSyncOrigin(kOrigin3, kResourceId3);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin1, kResourceId1);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin2, kResourceId2);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin3, kResourceId3);
   DisableOrigin(kOrigin3);
 
   EXPECT_EQ(kResourceId1, metadata_store()->GetResourceIdForOrigin(kOrigin1));
@@ -717,23 +670,18 @@ TEST_F(DriveMetadataStoreTest, MigrationFromV0) {
 
 TEST_F(DriveMetadataStoreTest, ResetOriginRootDirectory) {
   const GURL kOrigin1("chrome-extension://example1");
-  const GURL kOrigin2("chrome-extension://example2");
   const std::string kResourceId1("hoge");
   const std::string kResourceId2("fuga");
-  const std::string kResourceId3("piyo");
 
   InitializeDatabase();
   EXPECT_EQ(SYNC_STATUS_OK, SetLargestChangeStamp(1));
 
-  metadata_store()->AddBatchSyncOrigin(kOrigin1, kResourceId1);
-  metadata_store()->AddBatchSyncOrigin(kOrigin2, kResourceId2);
-  metadata_store()->MoveBatchSyncOriginToIncremental(kOrigin2);
-  VerifyBatchSyncOrigin(kOrigin1, kResourceId1);
-  VerifyIncrementalSyncOrigin(kOrigin2, kResourceId2);
+  metadata_store()->AddIncrementalSyncOrigin(kOrigin1, kResourceId1);
+  VerifyIncrementalSyncOrigin(kOrigin1, kResourceId1);
   VerifyReverseMap();
 
-  metadata_store()->SetOriginRootDirectory(kOrigin2, kResourceId3);
-  VerifyIncrementalSyncOrigin(kOrigin2, kResourceId3);
+  metadata_store()->SetOriginRootDirectory(kOrigin1, kResourceId2);
+  VerifyIncrementalSyncOrigin(kOrigin1, kResourceId2);
   VerifyReverseMap();
 }