SyncFS: store disabled origins in DriveMetadataStore

chrome/browser/sync_file_system/drive_metadata_store.cc

Index: chrome/browser/sync_file_system/drive_metadata_store.cc
diff --git a/chrome/browser/sync_file_system/drive_metadata_store.cc b/chrome/browser/sync_file_system/drive_metadata_store.cc
index 3831c43c5f96d026873c4346129f9d57baaf95fe..2598e7a62404bdc8dfbddd872d1ec3a98221622e 100644
--- a/chrome/browser/sync_file_system/drive_metadata_store.cc
+++ b/chrome/browser/sync_file_system/drive_metadata_store.cc
@@ -44,6 +44,7 @@ const char kDriveMetadataKeyPrefix[] = "METADATA: ";
 const char kMetadataKeySeparator = ' ';
 const char kDriveBatchSyncOriginKeyPrefix[] = "BSYNC_ORIGIN: ";
 const char kDriveIncrementalSyncOriginKeyPrefix[] = "ISYNC_ORIGIN: ";
+const char kDriveDisabledSyncOriginKeyPrefix[] = "DSYNC_ORIGIN: ";
 const size_t kDriveMetadataKeyPrefixLength = arraysize(kDriveMetadataKeyPrefix);
 
 const base::FilePath::CharType kV0FormatPathPrefix[] =
@@ -111,14 +112,22 @@ class DriveMetadataDB {
                              const DriveMetadata& metadata);
   SyncStatusCode DeleteEntry(const FileSystemURL& url);
 
+  // TODO(calvinlo): consolidate these state transition functions for sync
+  // origins like "UpdateSyncOrigin(GURL, SyncStatusEnum)". And manage origins
+  // in just one map like "Map<SyncStatusEnum, ResourceIDMap>".

[kinuko, 2013/03/16 23:16:51]

Can we file a bug for this?

[nhiroki, 2013/03/18 09:40:18]

Done.

   SyncStatusCode UpdateSyncOriginAsBatch(const GURL& origin,
                                          const std::string& resource_id);
   SyncStatusCode UpdateSyncOriginAsIncremental(const GURL& origin,
                                                const std::string& resource_id);
+  SyncStatusCode EnableOriginSync(const GURL& origin,
+                                  const std::string& resource_id);
+  SyncStatusCode DisableOriginSync(const GURL& origin,
+                                   const std::string& resource_id);
   SyncStatusCode RemoveOrigin(const GURL& origin);
 
   SyncStatusCode GetSyncOrigins(ResourceIDMap* batch_sync_origins,
-                                ResourceIDMap* incremental_sync_origins);
+                                ResourceIDMap* incremental_sync_origins,
+                                ResourceIDMap* disabled_sync_origins);

[kinuko, 2013/03/16 23:16:51]

Hmm, ok so we seem to use the term 'SyncOrigin' a lot in this class.  Then the
name 'EnableOriginSync' and 'DisableOriginSync' may look a bit inconsistent...
and returning disabled ones for the method 'GetSyncOrigins' feels a bit weird.

I feel we could just drop 'Sync' from most method names and can rename them to:

UpdateSyncOriginAs... -> UpdateOriginAs...Sync
GetSyncOrigins -> GetOrigins
EnableOriginSync -> EnableOrigin
DisableOriginSync -> DisableOrigin
IsDisabledSyncOrigin -> IsOriginDisabled

These would match better with following MetadataStore method names:

MetadataStore::GetEnabledOrigins()
MetadataStore::GetDisabledOrigins()

wdyt?

[nhiroki, 2013/03/18 09:40:18]

Looks good. (hopefully) I replaced them without any omission. Done.

 
  private:
   bool CalledOnValidThread() const {
@@ -139,6 +148,7 @@ struct DriveMetadataDBContents {
   std::string sync_root_directory_resource_id;
   DriveMetadataStore::ResourceIDMap batch_sync_origins;
   DriveMetadataStore::ResourceIDMap incremental_sync_origins;
+  DriveMetadataStore::ResourceIDMap disabled_sync_origins;
 };
 
 namespace {
@@ -152,6 +162,7 @@ SyncStatusCode InitializeDBOnFileThread(DriveMetadataDB* db,
   contents->metadata_map.clear();
   contents->batch_sync_origins.clear();
   contents->incremental_sync_origins.clear();
+  contents->disabled_sync_origins.clear();
 
   bool created = false;
   SyncStatusCode status = db->Initialize(&created);
@@ -169,9 +180,11 @@ SyncStatusCode InitializeDBOnFileThread(DriveMetadataDB* db,
   return db->ReadContents(contents);
 }
 
+
 // Returns a key string for the given batch sync origin.
 // For example, when |origin| is "http://www.example.com",
 // returns "BSYNC_ORIGIN: http://www.example.com".
+// TODO(calvinlo): consolidate these CreateKeyFor* functions.
 std::string CreateKeyForBatchSyncOrigin(const GURL& origin) {
   DCHECK(origin.is_valid());
   return kDriveBatchSyncOriginKeyPrefix + origin.spec();
@@ -185,6 +198,14 @@ std::string CreateKeyForIncrementalSyncOrigin(const GURL& origin) {
   return kDriveIncrementalSyncOriginKeyPrefix + origin.spec();
 }
 
+// Returns a key string for the given disabled sync origin.
+// For example, when |origin| is "http://www.example.com",
+// returns "DSYNC_ORIGIN: http://www.example.com".
+std::string CreateKeyForDisabledSyncOrigin(const GURL& origin) {
+  DCHECK(origin.is_valid());
+  return kDriveDisabledSyncOriginKeyPrefix + origin.spec();
+}
+
 void AddOriginsToVector(std::vector<GURL>* all_origins,
                         const DriveMetadataStore::ResourceIDMap& resource_map) {
   typedef DriveMetadataStore::ResourceIDMap::const_iterator itr;
@@ -240,6 +261,7 @@ void DriveMetadataStore::DidInitialize(const InitializationCallback& callback,
   sync_root_directory_resource_id_ = contents->sync_root_directory_resource_id;
   batch_sync_origins_.swap(contents->batch_sync_origins);
   incremental_sync_origins_.swap(contents->incremental_sync_origins);
+  disabled_sync_origins_.swap(contents->disabled_sync_origins);
   // |largest_changestamp_| is set to 0 for a fresh empty database.
   callback.Run(status, largest_changestamp_ <= 0);
 }
@@ -280,26 +302,31 @@ void DriveMetadataStore::RestoreSyncOrigins(
   DCHECK(CalledOnValidThread());
   ResourceIDMap* batch_sync_origins = new ResourceIDMap;
   ResourceIDMap* incremental_sync_origins = new ResourceIDMap;
+  ResourceIDMap* disabled_sync_origins = new ResourceIDMap;
   base::PostTaskAndReplyWithResult(
       file_task_runner_, FROM_HERE,
       base::Bind(&DriveMetadataDB::GetSyncOrigins,
                  base::Unretained(db_.get()),
                  batch_sync_origins,
-                 incremental_sync_origins),
+                 incremental_sync_origins,
+                 disabled_sync_origins),
       base::Bind(&DriveMetadataStore::DidRestoreSyncOrigins,
                  AsWeakPtr(), callback,
                  base::Owned(batch_sync_origins),
-                 base::Owned(incremental_sync_origins)));
+                 base::Owned(incremental_sync_origins),
+                 base::Owned(disabled_sync_origins)));
 }
 
 void DriveMetadataStore::DidRestoreSyncOrigins(
     const SyncStatusCallback& callback,
     ResourceIDMap* batch_sync_origins,
     ResourceIDMap* incremental_sync_origins,
+    ResourceIDMap* disabled_sync_origins,
     SyncStatusCode status) {
   DCHECK(CalledOnValidThread());
   DCHECK(batch_sync_origins);
   DCHECK(incremental_sync_origins);
+  DCHECK(disabled_sync_origins);
 
   db_status_ = status;
   if (status != SYNC_STATUS_OK) {
@@ -309,6 +336,7 @@ void DriveMetadataStore::DidRestoreSyncOrigins(
 
   batch_sync_origins_.swap(*batch_sync_origins);
   incremental_sync_origins_.swap(*incremental_sync_origins);
+  disabled_sync_origins_.swap(*disabled_sync_origins);
   callback.Run(status);
 }
 
@@ -420,11 +448,17 @@ bool DriveMetadataStore::IsIncrementalSyncOrigin(const GURL& origin) const {
   return ContainsKey(incremental_sync_origins_, origin);
 }
 
+bool DriveMetadataStore::IsDisabledSyncOrigin(const GURL& origin) const {
+  DCHECK(CalledOnValidThread());
+  return ContainsKey(disabled_sync_origins_, origin);
+}
+
 void DriveMetadataStore::AddBatchSyncOrigin(const GURL& origin,
                                             const std::string& resource_id) {
   DCHECK(CalledOnValidThread());
   DCHECK(!IsBatchSyncOrigin(origin));
   DCHECK(!IsIncrementalSyncOrigin(origin));
+  DCHECK(!IsDisabledSyncOrigin(origin));
   DCHECK_EQ(SYNC_STATUS_OK, db_status_);
 
   batch_sync_origins_.insert(std::make_pair(origin, resource_id));
@@ -441,6 +475,7 @@ void DriveMetadataStore::MoveBatchSyncOriginToIncremental(const GURL& origin) {
   DCHECK(CalledOnValidThread());
   DCHECK(IsBatchSyncOrigin(origin));
   DCHECK(!IsIncrementalSyncOrigin(origin));
+  DCHECK(!IsDisabledSyncOrigin(origin));
   DCHECK_EQ(SYNC_STATUS_OK, db_status_);
 
   std::map<GURL, std::string>::iterator found =
@@ -457,6 +492,60 @@ void DriveMetadataStore::MoveBatchSyncOriginToIncremental(const GURL& origin) {
   batch_sync_origins_.erase(found);
 }
 
+void DriveMetadataStore::EnableOriginSync(
+    const GURL& origin,
+    const SyncStatusCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsDisabledSyncOrigin(origin));
+  DCHECK(!IsBatchSyncOrigin(origin));
+  DCHECK(!IsIncrementalSyncOrigin(origin));

[kinuko, 2013/03/16 23:16:51]

Nit-picky comment: If we had a crash after an extension is disabled (and the
status change is persisted in extension system) but before the changes are
successfully written into the metadata store, this method may get called without
meeting these conditions, am I correct?  DCHECKs aren't hit in release build but
if something goes wrong enabling the extension may keep failing in debug build?

[nhiroki, 2013/03/18 09:40:18]

You are right. I think it's not needed to check them strictly here. I changed
these checks to simple if-statements.

+
+  std::map<GURL, std::string>::iterator found =
+      disabled_sync_origins_.find(origin);
+  batch_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::EnableOriginSync,
+                 base::Unretained(db_.get()), origin, found->second),
+      base::Bind(&DriveMetadataStore::DidChangeOrigin, AsWeakPtr(), callback));
+
+  disabled_sync_origins_.erase(found);
+}
+
+void DriveMetadataStore::DisableOriginSync(
+    const GURL& origin,
+    const SyncStatusCallback& callback) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(IsBatchSyncOrigin(origin) || IsIncrementalSyncOrigin(origin));
+  DCHECK(!IsDisabledSyncOrigin(origin));

[kinuko, 2013/03/16 23:16:51]

Ditto.

[nhiroki, 2013/03/18 09:40:18]

Done.

+
+  std::string resource_id;
+  std::map<GURL, std::string>::iterator found =
+      batch_sync_origins_.find(origin);
+  if (found != batch_sync_origins_.end()) {
+    resource_id = found->second;
+    batch_sync_origins_.erase(found);
+  }
+
+  found = incremental_sync_origins_.find(origin);
+  if (found != incremental_sync_origins_.end()) {
+    resource_id = found->second;
+    incremental_sync_origins_.erase(found);
+  }
+
+  DCHECK(!resource_id.empty());
+  disabled_sync_origins_.insert(std::make_pair(origin, resource_id));
+
+  // Store a pair of |origin| and |resource_id| in the DB.
+  base::PostTaskAndReplyWithResult(
+      file_task_runner_, FROM_HERE,
+      base::Bind(&DriveMetadataDB::DisableOriginSync,
+                 base::Unretained(db_.get()), origin, resource_id),
+      base::Bind(&DriveMetadataStore::DidChangeOrigin, AsWeakPtr(), callback));
+}
+
 void DriveMetadataStore::RemoveOrigin(
     const GURL& origin,
     const SyncStatusCallback& callback) {
@@ -465,15 +554,16 @@ void DriveMetadataStore::RemoveOrigin(
   metadata_map_.erase(origin);
   batch_sync_origins_.erase(origin);
   incremental_sync_origins_.erase(origin);
+  disabled_sync_origins_.erase(origin);
 
   base::PostTaskAndReplyWithResult(
       file_task_runner_, FROM_HERE,
       base::Bind(&DriveMetadataDB::RemoveOrigin,
                  base::Unretained(db_.get()), origin),
-      base::Bind(&DriveMetadataStore::DidRemoveOrigin, AsWeakPtr(), callback));
+      base::Bind(&DriveMetadataStore::DidChangeOrigin, AsWeakPtr(), callback));
 }
 
-void DriveMetadataStore::DidRemoveOrigin(
+void DriveMetadataStore::DidChangeOrigin(
     const SyncStatusCallback& callback,
     SyncStatusCode status) {
   UpdateDBStatus(status);
@@ -545,7 +635,9 @@ SyncStatusCode DriveMetadataStore::GetToBeFetchedFiles(
 std::string DriveMetadataStore::GetResourceIdForOrigin(
     const GURL& origin) const {
   DCHECK(CalledOnValidThread());
-  DCHECK(IsBatchSyncOrigin(origin) || IsIncrementalSyncOrigin(origin));
+  DCHECK(IsBatchSyncOrigin(origin) ||
+         IsIncrementalSyncOrigin(origin) ||
+         IsDisabledSyncOrigin(origin));
 
   ResourceIDMap::const_iterator found = incremental_sync_origins_.find(origin);
   if (found != incremental_sync_origins_.end())
@@ -555,17 +647,28 @@ std::string DriveMetadataStore::GetResourceIdForOrigin(
   if (found != batch_sync_origins_.end())
     return found->second;
 
+  found = disabled_sync_origins_.find(origin);
+  if (found != disabled_sync_origins_.end())
+    return found->second;
+
   NOTREACHED();
   return std::string();
 }
 
-void DriveMetadataStore::GetAllOrigins(std::vector<GURL>* origins) {
+void DriveMetadataStore::GetEnabledOrigins(std::vector<GURL>* origins) {
+  origins->clear();
   origins->reserve(batch_sync_origins_.size() +
                    incremental_sync_origins_.size());
   AddOriginsToVector(origins, batch_sync_origins_);
   AddOriginsToVector(origins, incremental_sync_origins_);
 }
 
+void DriveMetadataStore::GetDisabledOrigins(std::vector<GURL>* origins) {
+  origins->clear();
+  origins->reserve(disabled_sync_origins_.size());
+  AddOriginsToVector(origins, disabled_sync_origins_);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 
 DriveMetadataDB::DriveMetadataDB(const base::FilePath& base_dir,
@@ -646,7 +749,8 @@ SyncStatusCode DriveMetadataDB::ReadContents(
   }
 
   SyncStatusCode status = GetSyncOrigins(&contents->batch_sync_origins,
-                                         &contents->incremental_sync_origins);
+                                         &contents->incremental_sync_origins,
+                                         &contents->disabled_sync_origins);
   if (status != SYNC_STATUS_OK &&
       status != SYNC_DATABASE_ERROR_NOT_FOUND)
     return status;
@@ -718,6 +822,9 @@ SyncStatusCode DriveMetadataDB::MigrateFromVersion0Database() {
   //
   //   key: "ISYNC_ORIGIN: " + <URL string of a incremental sync origin>
   //   value: <Resource ID of the drive directory for the origin>
+  //
+  //   key: "DSYNC_ORIGIN: " + <URL string of a disabled sync origin>
+  //   value: <Resource ID of the drive directory for the origin>
 
   leveldb::WriteBatch write_batch;
   write_batch.Put(kDatabaseVersionKey,
@@ -820,18 +927,48 @@ SyncStatusCode DriveMetadataDB::UpdateSyncOriginAsIncremental(
 
   leveldb::WriteBatch batch;
   batch.Delete(CreateKeyForBatchSyncOrigin(origin));
+  batch.Delete(CreateKeyForDisabledSyncOrigin(origin));
   batch.Put(CreateKeyForIncrementalSyncOrigin(origin), resource_id);
   leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
 
   return LevelDBStatusToSyncStatusCode(status);
 }
 
+SyncStatusCode DriveMetadataDB::EnableOriginSync(
+    const GURL& origin, const std::string& resource_id) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(db_.get());
+
+  leveldb::WriteBatch batch;
+  batch.Delete(CreateKeyForIncrementalSyncOrigin(origin));
+  batch.Delete(CreateKeyForDisabledSyncOrigin(origin));
+  batch.Put(CreateKeyForBatchSyncOrigin(origin), resource_id);
+  leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
+
+  return LevelDBStatusToSyncStatusCode(status);
+}
+
+SyncStatusCode DriveMetadataDB::DisableOriginSync(
+    const GURL& origin, const std::string& resource_id) {
+  DCHECK(CalledOnValidThread());
+  DCHECK(db_.get());
+
+  leveldb::WriteBatch batch;
+  batch.Delete(CreateKeyForBatchSyncOrigin(origin));
+  batch.Delete(CreateKeyForIncrementalSyncOrigin(origin));
+  batch.Put(CreateKeyForDisabledSyncOrigin(origin), resource_id);
+  leveldb::Status status = db_->Write(leveldb::WriteOptions(), &batch);
+
+  return LevelDBStatusToSyncStatusCode(status);
+}
+
 SyncStatusCode DriveMetadataDB::RemoveOrigin(const GURL& origin_to_remove) {
   DCHECK(CalledOnValidThread());
 
   leveldb::WriteBatch batch;
-  batch.Delete(kDriveBatchSyncOriginKeyPrefix + origin_to_remove.spec());
-  batch.Delete(kDriveIncrementalSyncOriginKeyPrefix + origin_to_remove.spec());
+  batch.Delete(CreateKeyForBatchSyncOrigin(origin_to_remove));
+  batch.Delete(CreateKeyForIncrementalSyncOrigin(origin_to_remove));
+  batch.Delete(CreateKeyForDisabledSyncOrigin(origin_to_remove));
 
   scoped_ptr<leveldb::Iterator> itr(db_->NewIterator(leveldb::ReadOptions()));
   std::string metadata_key = kDriveMetadataKeyPrefix + origin_to_remove.spec();
@@ -853,7 +990,8 @@ SyncStatusCode DriveMetadataDB::RemoveOrigin(const GURL& origin_to_remove) {
 
 SyncStatusCode DriveMetadataDB::GetSyncOrigins(
     ResourceIDMap* batch_sync_origins,
-    ResourceIDMap* incremental_sync_origins) {
+    ResourceIDMap* incremental_sync_origins,
+    ResourceIDMap* disabled_sync_origins) {
   DCHECK(CalledOnValidThread());
   DCHECK(db_.get());
 
@@ -889,6 +1027,21 @@ SyncStatusCode DriveMetadataDB::GetSyncOrigins(
     DCHECK(result);
   }
 
+  // Get disabled sync origins from the DB.
+  for (itr->Seek(kDriveDisabledSyncOriginKeyPrefix);
+       itr->Valid(); itr->Next()) {
+    std::string key = itr->key().ToString();
+    if (!StartsWithASCII(key, kDriveDisabledSyncOriginKeyPrefix, true))
+      break;
+    GURL origin(std::string(
+        key.begin() + arraysize(kDriveDisabledSyncOriginKeyPrefix) - 1,
+        key.end()));
+    DCHECK(origin.is_valid());
+    bool result = disabled_sync_origins->insert(
+        std::make_pair(origin, itr->value().ToString())).second;
+    DCHECK(result);
+  }
+
   return SYNC_STATUS_OK;
 }