Add to be fetched files instead of DriveMetadataStore.batch_sync_origins_

chrome/browser/sync_file_system/drive_file_sync_service.cc

Index: chrome/browser/sync_file_system/drive_file_sync_service.cc
diff --git a/chrome/browser/sync_file_system/drive_file_sync_service.cc b/chrome/browser/sync_file_system/drive_file_sync_service.cc
index 2958e356e9ab251fb91512978f3e0ce514cf8c31..733be9d71e6c2723d744938c9ec34e23473609b5 100644
--- a/chrome/browser/sync_file_system/drive_file_sync_service.cc
+++ b/chrome/browser/sync_file_system/drive_file_sync_service.cc
@@ -435,15 +435,6 @@ void DriveFileSyncService::DidInitializeMetadataStore(
 
   largest_fetched_changestamp_ = metadata_store_->GetLargestChangeStamp();
 
-  // Mark all the batch sync origins as 'pending' so that we can start
-  // batch sync when we're ready.
-  for (std::map<GURL, std::string>::const_iterator itr =
-           metadata_store_->batch_sync_origins().begin();
-       itr != metadata_store_->batch_sync_origins().end();
-       ++itr) {
-    pending_batch_sync_origins_.insert(itr->first);
-  }

[kinuko, 2013/05/20 02:42:08]

Drive-by question (probaby to tzik): when are we adding existing batch sync
origins (that are not yet migrated in drive_metadata) to
pending_batch_sync_origins? (Just want to make sure)

It'be nice how batch sync origin changes before and after this patch.

[calvinlo, 2013/05/20 03:53:14]

I'm not sure I understand your question. If you're referring to why removing
this code block doesn't break the current behavior than I believe that I already
put the answer in the issue description.

Instead of restoring whole origins back into pending_batch_sync_origins_ the
files belonging to that origin have their to_be_fetched flag set as true. They
are then restored in lines 447 - 460. The files have their to_be_fetch bit set
in DidGetDirectoryContentForBatchSync. 

In the case that Chrome crashes before the files for the origin are added as to
be fetched, they will only be picked up again when the extension is reloaded and
the origin is added as if it was new.

Does this answer your question?

[kinuko, 2013/05/20 04:06:22]

I think I'm asking about the latter case, and my previous comment (for line
2194) was about my concern that it doesn't look it's always added back at a
quick glance.

We may add back the origin at line 753 or at line 2194, but line 752 will return
false if the origin is already in batch-sync-origin list and we only go through
line 2194 if the origin's resource_id has not been fetched yet (but it's usually
not true for already-registered batch sync origins)? I'm happy to be corrected.

[tzik, 2013/05/20 04:27:59]

Right, if a origin is already registered as a BatchSyncOrigin,
pending_batch_sync_origin_ will not work.
I think we should just drop BatchSyncOrigin from DriveMetadataStore soon after
the database is initialized.

[calvinlo, 2013/05/20 04:31:54]

It should get added back at line 753. Line 752 should return false because the
origin isn't in DriveMetadataStore.batch_sync_origins_. This is true because the
original line 762 was removed. Effectively this CL renders
DriveMetadataStore.batch_sync_origins_ unused except on line 852 where it's use
temporarily to move the origin directly to incremental status.

[kinuko, 2013/05/20 04:41:47]

For a fresh install it should work, but Chrome can get auto-updated while the DB
may have batch_sync_origins, can't it?

-
   DriveMetadataStore::URLAndDriveMetadataList to_be_fetched_files;
   status = metadata_store_->GetToBeFetchedFiles(&to_be_fetched_files);
   DCHECK_EQ(SYNC_STATUS_OK, status);
@@ -559,7 +550,8 @@ void DriveFileSyncService::DoEnableOriginForTrackingChanges(
   }
 
   metadata_store_->EnableOrigin(origin, callback);
-  pending_batch_sync_origins_.insert(origin);
+  pending_batch_sync_origins_.insert(
+      *metadata_store_->disabled_origins().find(origin));
 }
 
 void DriveFileSyncService::DoDisableOriginForTrackingChanges(
@@ -702,8 +694,9 @@ void DriveFileSyncService::UpdateRegisteredOrigins() {
     } else if (metadata_store_->IsOriginDisabled(origin) &&
                extension_service->IsExtensionEnabled(extension_id)) {
       // Extension has been re-enabled.
+      pending_batch_sync_origins_.insert(
+          *metadata_store_->disabled_origins().find(origin));
       metadata_store_->EnableOrigin(origin, base::Bind(&EmptyStatusCallback));
-      pending_batch_sync_origins_.insert(origin);
     }
   }
 }
@@ -711,16 +704,12 @@ void DriveFileSyncService::UpdateRegisteredOrigins() {
 void DriveFileSyncService::StartBatchSync(
     const SyncStatusCallback& callback) {
   DCHECK(GetCurrentState() == REMOTE_SERVICE_OK || may_have_unfetched_changes_);
+  DCHECK(!pending_batch_sync_origins_.empty());
 
-  if (pending_batch_sync_origins_.empty()) {
-    callback.Run(SYNC_STATUS_OK);
-    return;
-  }
-
-  GURL origin = *pending_batch_sync_origins_.begin();
-  pending_batch_sync_origins_.erase(pending_batch_sync_origins_.begin());
-  std::string resource_id = metadata_store_->GetResourceIdForOrigin(origin);
+  GURL origin = pending_batch_sync_origins_.begin()->first;
+  std::string resource_id = pending_batch_sync_origins_.begin()->second;
   DCHECK(!resource_id.empty());
+  pending_batch_sync_origins_.erase(pending_batch_sync_origins_.begin());
 
   DCHECK(!metadata_store_->IsOriginDisabled(origin));
 
@@ -758,9 +747,10 @@ void DriveFileSyncService::DidGetDriveDirectoryForOrigin(
   }
 
   // Add this origin to batch sync origin if it hasn't been already.
+  // TODO(calvinlo): Added files to file metadata mapping, this can be removed
+  // next when batch_sync_origins is removed from DriveMetadataStore.
   if (!metadata_store_->IsKnownOrigin(origin)) {
-    metadata_store_->AddBatchSyncOrigin(origin, resource_id);
-    pending_batch_sync_origins_.insert(origin);
+    pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id));
   }
 
   callback.Run(SYNC_STATUS_OK);
@@ -788,7 +778,7 @@ void DriveFileSyncService::DidGetLargestChangeStampForBatchSync(
     google_apis::GDataErrorCode error,
     int64 largest_changestamp) {
   if (error != google_apis::HTTP_SUCCESS) {
-    pending_batch_sync_origins_.insert(origin);
+    pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id));
     callback.Run(GDataErrorCodeToSyncStatusCodeWrapper(error));
     return;
   }
@@ -806,17 +796,19 @@ void DriveFileSyncService::DidGetLargestChangeStampForBatchSync(
                  AsWeakPtr(),
                  callback,
                  origin,
+                 resource_id,
                  largest_changestamp));
 }
 
 void DriveFileSyncService::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) {
   if (error != google_apis::HTTP_SUCCESS) {
-    pending_batch_sync_origins_.insert(origin);
+    pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id));
     callback.Run(GDataErrorCodeToSyncStatusCodeWrapper(error));
     return;
   }
@@ -824,8 +816,22 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
   typedef ScopedVector<google_apis::ResourceEntry>::const_iterator iterator;
   for (iterator itr = feed->entries().begin();
        itr != feed->entries().end(); ++itr) {
-    AppendRemoteChange(origin, **itr, largest_changestamp,
+    const google_apis::ResourceEntry& entry = **itr;
+    AppendRemoteChange(origin, entry, largest_changestamp,
                        RemoteChangeHandler::REMOTE_SYNC_TYPE_BATCH);
+
+    // Save to be fetched file to DB for restore in case of crash.
+    DriveMetadata metadata;
+    metadata.set_resource_id(entry.resource_id());
+    metadata.set_md5_checksum(entry.file_md5());
+    metadata.set_conflicted(false);
+    metadata.set_to_be_fetched(true);
+
+    base::FilePath path = TitleToPath(entry.title());
+    fileapi::FileSystemURL url(CreateSyncableFileSystemURL(
+        origin, kServiceName, path));
+    metadata_store_->UpdateEntry(url, metadata,
+                                 base::Bind(&EmptyStatusCallback));
   }
 

[tzik, 2013/05/20 04:27:59]

If Chrome crashed around here, the database will turn to wrong state. That is,
the database will contain a DriveMetadata without know origin.

Putting all these change into single WriteBatch will solve the problem.
Could you leave a TODO to do so?

[calvinlo, 2013/05/20 13:48:58]

Done.

   GURL next_feed_url;
@@ -836,16 +842,15 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
                    AsWeakPtr(),
                    callback,
                    origin,
+                   resource_id,
                    largest_changestamp));
     return;
   }
 
-  // Move |origin| to the incremental sync origin set if the origin has no file.
-  if (metadata_store_->IsBatchSyncOrigin(origin) &&
-      !remote_change_handler_.HasChangesForOrigin(origin)) {
-    metadata_store_->MoveBatchSyncOriginToIncremental(origin);
-  }
-
+  // TODO(calvinlo): Eliminate batch_sync_origins so
+  // MoveBatchSyncOriginToIncremental() happens directly.
+  metadata_store_->AddBatchSyncOrigin(origin, resource_id);
+  metadata_store_->MoveBatchSyncOriginToIncremental(origin);
   may_have_unfetched_changes_ = true;
   callback.Run(SYNC_STATUS_OK);
 }
@@ -2186,7 +2191,7 @@ void DriveFileSyncService::DidEnsureOriginRoot(
       metadata_store_->IsKnownOrigin(origin)) {
     metadata_store_->SetOriginRootDirectory(origin, resource_id);
     if (metadata_store_->IsBatchSyncOrigin(origin))
-      pending_batch_sync_origins_.insert(origin);
+      pending_batch_sync_origins_.insert(std::make_pair(origin, resource_id));

[kinuko, 2013/05/20 03:42:22]

To me it looks we only (re-)add existing batch sync origins to
pending_batch_sync_origins when GetResourceIdForOrigin() returns an empty
resource_id for the origin when the app is reloaded?  I could be wrong.

   }
   callback.Run(status, resource_id);
 }