Deprecate DriveMetadataStore.batch_sync_origins

Deprecate batch_sync_origins from DriveMetadataStore so batch
origins are no longer duplicated in both
DriveFileSyncService.pending_batch_sync_origins and
DriveMetadataStore.batch_sync_origins_;

BUG=229764
TEST=DriveMetadataStoreTest.DeprecateBatchSyncOrigins
+ Existing unit_tests --gtest_filter=DriveFileSync*
+ Existing unit_tests --gtest_filter=DriveMeta*

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

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

chrome/browser/sync_file_system/drive_file_sync_service.cc

@@ -381,19 +381,6 @@ void DriveFileSyncService::DidInitializeMetadataStore(
     return;
   }
 
-  // TODO(calvinlo): Move the code to delete legacy batch_sync_origin keys from
-  // DB into DriveMetadataStore.InitializeDBOnFileThread.
-  std::vector<GURL> batch_origins_to_delete;
-  typedef std::map<GURL, std::string>::const_iterator origin_itr;
-  for (origin_itr itr = metadata_store_->batch_sync_origins().begin();
-       itr != metadata_store_->batch_sync_origins().end(); ++itr) {
-    batch_origins_to_delete.push_back(itr->first);
-  }
-  for (std::vector<GURL>::const_iterator itr = batch_origins_to_delete.begin();
-       itr != batch_origins_to_delete.end(); ++itr) {
-    metadata_store_->RemoveOrigin(*itr, base::Bind(&EmptyStatusCallback));
-  }
-
   DCHECK(pending_batch_sync_origins_.empty());
 
   UpdateRegisteredOrigins();
@@ -615,8 +602,7 @@ void DriveFileSyncService::DoApplyLocalChange(
     return;
   }
 
-  if (!metadata_store_->IsIncrementalSyncOrigin(url.origin()) &&
-      !metadata_store_->IsBatchSyncOrigin(url.origin())) {
+  if (!metadata_store_->IsIncrementalSyncOrigin(url.origin())) {
     // We may get called by LocalFileSyncService to sync local changes
     // for the origins that are disabled.
     DVLOG(1) << "Got request for stray origin: " << url.origin().spec();
@@ -636,7 +622,6 @@ void DriveFileSyncService::UpdateRegisteredOrigins() {
   ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   DCHECK(pending_batch_sync_origins_.empty());
-  DCHECK(metadata_store_->batch_sync_origins().empty());
   if (!extension_service)
     return;
 
@@ -1653,8 +1638,7 @@ bool DriveFileSyncService::GetOriginForEntry(
     }
     DCHECK(origin.is_valid());
 
-    if (!metadata_store_->IsBatchSyncOrigin(origin) &&
-        !metadata_store_->IsIncrementalSyncOrigin(origin))
+    if (!metadata_store_->IsIncrementalSyncOrigin(origin))
       continue;
     std::string resource_id(metadata_store_->GetResourceIdForOrigin(origin));
     if (resource_id.empty())

chrome/browser/sync_file_system/drive_metadata_store.h

@@ -24,6 +24,10 @@ namespace base {
 class SequencedTaskRunner;
 }
 
+namespace leveldb {
+class DB;
+}
+
 class GURL;
 
 namespace sync_file_system {
@@ -79,14 +83,9 @@ class DriveMetadataStore
   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.
+  // Returns true if |origin| is an incremental sync or disabled origin.
   bool IsKnownOrigin(const GURL& origin) const;
 
-  // Returns true if |origin| is a batch sync origin, i.e. the origin's entire
-  // file list hasn't been fully fetched and processed yet.
-  bool IsBatchSyncOrigin(const GURL& origin) const;
-
   // Returns true if |origin| is an incremental sync origin, i.e. the origin's
   // entire file list has been cached and is ready to apply changes
   // incrementally.
@@ -118,8 +117,8 @@ class DriveMetadataStore
   SyncStatusCode GetToBeFetchedFiles(URLAndDriveMetadataList* list) const;
 
   // Returns resource id for |origin|.
-  // This may return an empty string if |origin| is not a batch, incremental
-  // or disabled origin.
+  // This may return an empty string if |origin| is not a incremental or
+  // disabled origin.
   std::string GetResourceIdForOrigin(const GURL& origin) const;
 
   const std::string& sync_root_directory() const {
@@ -127,11 +126,6 @@ class DriveMetadataStore
     return sync_root_directory_resource_id_;
   }
 
-  const ResourceIdByOrigin& batch_sync_origins() const {
-    DCHECK(CalledOnValidThread());
-    return batch_sync_origins_;
-  }
-
   const ResourceIdByOrigin& incremental_sync_origins() const {
     DCHECK(CalledOnValidThread());
     return incremental_sync_origins_;
@@ -142,8 +136,8 @@ class DriveMetadataStore
     return disabled_origins_;
   }
 
-  // Returns all tracked origins. i.e. Union of batch_sync_origins_,
-  // incremental_sync_origins_ and disabled_origins_.
+  // Returns all tracked origins. i.e. incremental_sync_origins_ and
+  // disabled_origins_.
   void GetAllOrigins(std::vector<GURL>* origins);
 
   // Maps |resource_id| to corresponding |origin|.
@@ -155,15 +149,6 @@ 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);
@@ -180,10 +165,10 @@ class DriveMetadataStore
                                    SyncStatusCode status);
   void RestoreOrigins(const SyncStatusCallback& callback);
   void DidRestoreOrigins(const SyncStatusCallback& callback,
-                         ResourceIdByOrigin* batch_sync_origins,
                          ResourceIdByOrigin* incremental_sync_origins,
                          ResourceIdByOrigin* disabled_origins,
                          SyncStatusCode status);
+  leveldb::DB* GetDBInstanceForTesting();
 
   scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
   scoped_ptr<DriveMetadataDB> db_;
@@ -193,7 +178,6 @@ class DriveMetadataStore
   MetadataMap metadata_map_;
 
   std::string sync_root_directory_resource_id_;
-  ResourceIdByOrigin batch_sync_origins_;
   ResourceIdByOrigin incremental_sync_origins_;
   ResourceIdByOrigin disabled_origins_;
 

chrome/browser/sync_file_system/drive_metadata_store_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/message_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
+#include "base/string_util.h"
 #include "base/threading/thread.h"
 #include "chrome/browser/sync_file_system/drive_file_sync_service.h"
 #include "chrome/browser/sync_file_system/sync_file_system.pb.h"
@@ -132,10 +133,8 @@ class DriveMetadataStoreTest : public testing::Test {
 
   void DropSyncOriginsInStore() {
     EXPECT_TRUE(ui_task_runner_->RunsTasksOnCurrentThread());
-    drive_metadata_store_->batch_sync_origins_.clear();
     drive_metadata_store_->incremental_sync_origins_.clear();
     drive_metadata_store_->disabled_origins_.clear();
-    EXPECT_TRUE(drive_metadata_store_->batch_sync_origins().empty());
     EXPECT_TRUE(drive_metadata_store_->incremental_sync_origins().empty());
     EXPECT_TRUE(drive_metadata_store_->disabled_origins().empty());
   }
@@ -216,14 +215,12 @@ class DriveMetadataStoreTest : public testing::Test {
   }
 
   void VerifyUntrackedOrigin(const GURL& origin) {
-    EXPECT_FALSE(metadata_store()->IsBatchSyncOrigin(origin));
     EXPECT_FALSE(metadata_store()->IsIncrementalSyncOrigin(origin));
     EXPECT_FALSE(metadata_store()->IsOriginDisabled(origin));
   }
 
   void VerifyIncrementalSyncOrigin(const GURL& origin,
                                    const std::string& resource_id) {
-    EXPECT_FALSE(metadata_store()->IsBatchSyncOrigin(origin));
     EXPECT_TRUE(metadata_store()->IsIncrementalSyncOrigin(origin));
     EXPECT_FALSE(metadata_store()->IsOriginDisabled(origin));
     EXPECT_EQ(resource_id,
@@ -233,7 +230,6 @@ class DriveMetadataStoreTest : public testing::Test {
 
   void VerifyDisabledOrigin(const GURL& origin,
                             const std::string& resource_id) {
-    EXPECT_FALSE(metadata_store()->IsBatchSyncOrigin(origin));
     EXPECT_FALSE(metadata_store()->IsIncrementalSyncOrigin(origin));
     EXPECT_TRUE(metadata_store()->IsOriginDisabled(origin));
     EXPECT_EQ(resource_id,
@@ -248,13 +244,15 @@ class DriveMetadataStoreTest : public testing::Test {
     return drive_metadata_store_.get();
   }
 
+  leveldb::DB* metadata_db() {
+    return drive_metadata_store_->GetDBInstanceForTesting();
+  }
+
   const DriveMetadataStore::MetadataMap& metadata_map() {
     return drive_metadata_store_->metadata_map_;
   }
 
   void VerifyReverseMap() {
-    const ResourceIdByOrigin& batch_sync_origins =
-        drive_metadata_store_->batch_sync_origins_;
     const ResourceIdByOrigin& incremental_sync_origins =
         drive_metadata_store_->incremental_sync_origins_;
     const ResourceIdByOrigin& disabled_origins =
@@ -262,13 +260,10 @@ class DriveMetadataStoreTest : public testing::Test {
     const OriginByResourceId& origin_by_resource_id =
         drive_metadata_store_->origin_by_resource_id_;
 
-    size_t expected_size =
-        batch_sync_origins.size() + incremental_sync_origins.size() +
-        disabled_origins.size();
+    size_t expected_size = incremental_sync_origins.size() +
+                           disabled_origins.size();
     size_t actual_size = origin_by_resource_id.size();
     EXPECT_EQ(expected_size, actual_size);
-    EXPECT_TRUE(VerifyReverseMapInclusion(batch_sync_origins,
-                                          origin_by_resource_id));
     EXPECT_TRUE(VerifyReverseMapInclusion(incremental_sync_origins,
                                           origin_by_resource_id));
     EXPECT_TRUE(VerifyReverseMapInclusion(disabled_origins,
@@ -534,7 +529,6 @@ TEST_F(DriveMetadataStoreTest, RemoveOrigin) {
   InitializeDatabase();
 
   // 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));
@@ -652,7 +646,6 @@ TEST_F(DriveMetadataStoreTest, MigrationFromV0) {
 
   EXPECT_EQ(1, metadata_store()->GetLargestChangeStamp());
   EXPECT_EQ(kSyncRootResourceId, metadata_store()->sync_root_directory());
-  EXPECT_EQ(kResourceId1, metadata_store()->GetResourceIdForOrigin(kOrigin1));
   EXPECT_EQ(kResourceId2, metadata_store()->GetResourceIdForOrigin(kOrigin2));
 
   DriveMetadata metadata;
@@ -668,6 +661,42 @@ TEST_F(DriveMetadataStoreTest, MigrationFromV0) {
   VerifyReverseMap();
 }
 
+TEST_F(DriveMetadataStoreTest, DeprecateBatchSyncOrigins) {
+  // Make sure that previously saved batch sync origins were deleted from the DB
+  // as they are no longer used.
+  const char kDriveBatchSyncOriginKeyPrefix[] = "BSYNC_ORIGIN: ";
+  const GURL kOrigin1("chrome-extension://example1");
+  const std::string kResourceId1("hoge");
+
+  // Purposely add in an old batch sync origin (from previous DB version).
+  {
+    leveldb::Options options;
+    options.create_if_missing = true;
+    leveldb::DB* db_ptr = NULL;
+    std::string db_dir = fileapi::FilePathToString(
+        base_dir().Append(DriveMetadataStore::kDatabaseName));
+    leveldb::DB::Open(options, db_dir, &db_ptr);
+    scoped_ptr<leveldb::DB> db(db_ptr);
+    leveldb::WriteOptions write_options;
+    db->Put(write_options,
+            kDriveBatchSyncOriginKeyPrefix + kOrigin1.spec(), kResourceId1);
+  }
+
+  InitializeDatabase();
+
+  // Confirm no batch sync origins remain after InitializeDatabase.
+  scoped_ptr<leveldb::Iterator> itr(metadata_db()->NewIterator(
+      leveldb::ReadOptions()));
+  int batch_origins_found = 0;
+  for (itr->Seek(kDriveBatchSyncOriginKeyPrefix); itr->Valid(); itr->Next()) {
+    std::string key = itr->key().ToString();
+    if (!StartsWithASCII(key, kDriveBatchSyncOriginKeyPrefix, true))
+      break;
+    batch_origins_found++;
+  }
+  EXPECT_EQ(0, batch_origins_found);
+}
+
 TEST_F(DriveMetadataStoreTest, ResetOriginRootDirectory) {
   const GURL kOrigin1("chrome-extension://example1");
   const std::string kResourceId1("hoge");