Add to be fetched files instead of DriveMetadataStore.batch_sync_origins_

chrome/browser/sync_file_system/drive_file_sync_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/sync_file_system/drive_file_sync_service.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 417 matching lines @@
     callback.Run(status);
     return;
   }
 
   DCHECK(pending_batch_sync_origins_.empty());
 
   UpdateRegisteredOrigins();
 
   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);
   typedef DriveMetadataStore::URLAndDriveMetadataList::const_iterator iterator;
   for (iterator itr = to_be_fetched_files.begin();
        itr != to_be_fetched_files.end(); ++itr) {
     const FileSystemURL& url = itr->first;
     const DriveMetadata& metadata = itr->second;
     const std::string& resource_id = metadata.resource_id();
 
     SyncFileType file_type = SYNC_FILE_TYPE_FILE;
     if (metadata.type() == DriveMetadata::RESOURCE_TYPE_FOLDER)
       file_type = SYNC_FILE_TYPE_DIRECTORY;
     AppendFetchChange(url.origin(), url.path(), resource_id, file_type);

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

Could you add a check to ensure the origin() is incremental sync origin?
And if not, drop the change from the database.
# This is a fix for the comment around line 836.

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

Done.

   }
 
   if (!sync_root_resource_id().empty())
     api_util_->EnsureSyncRootIsNotInMyDrive(sync_root_resource_id());
 
   callback.Run(status);
   may_have_unfetched_changes_ = true;
 
   google_apis::DriveNotificationManager* drive_notification_manager =
       google_apis::DriveNotificationManagerFactory::GetForProfile(profile_);
@@ skipping 81 matching lines @@
 
 void DriveFileSyncService::DoEnableOriginForTrackingChanges(
     const GURL& origin,
     const SyncStatusCallback& callback) {
   if (!metadata_store_->IsOriginDisabled(origin)) {
     callback.Run(SYNC_STATUS_OK);
     return;
   }
 
   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(
     const GURL& origin,
     const SyncStatusCallback& callback) {
   if (!metadata_store_->IsBatchSyncOrigin(origin) &&
       !metadata_store_->IsIncrementalSyncOrigin(origin)) {
     callback.Run(SYNC_STATUS_OK);
     return;
   }
@@ skipping 122 matching lines @@
       // Extension has been uninstalled.
       UninstallOrigin(origin, base::Bind(&EmptyStatusCallback));
     } else if ((metadata_store_->IsBatchSyncOrigin(origin) ||
                 metadata_store_->IsIncrementalSyncOrigin(origin)) &&
                !extension_service->IsExtensionEnabled(extension_id)) {
       // Extension has been disabled.
       metadata_store_->DisableOrigin(origin, base::Bind(&EmptyStatusCallback));
     } 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);
     }
   }
 }
 
 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();
+  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());
-  std::string resource_id = metadata_store_->GetResourceIdForOrigin(origin);
-  DCHECK(!resource_id.empty());
 
   DCHECK(!metadata_store_->IsOriginDisabled(origin));
 
   DVLOG(1) << "Start batch sync for:" << origin.spec();
 
   api_util_->GetLargestChangeStamp(
       base::Bind(&DriveFileSyncService::DidGetLargestChangeStampForBatchSync,
                  AsWeakPtr(),
                  callback,
                  origin,
@@ skipping 17 matching lines @@
             AsWeakPtr(), origin, callback));
     return;
   }
 
   if (status != SYNC_STATUS_OK) {
     callback.Run(status);
     return;
   }
 
   // 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);
 }
 
 void DriveFileSyncService::DidUninstallOrigin(
     const GURL& origin,
     const SyncStatusCallback& callback,
     google_apis::GDataErrorCode error) {
   SyncStatusCode status = GDataErrorCodeToSyncStatusCodeWrapper(error);
   if (status != SYNC_STATUS_OK) {
     callback.Run(status);
     return;
   }
 
   // Origin directory has been removed so it's now safe to remove the origin
   // from the metadata store.
   DoUnregisterOriginForTrackingChanges(origin, callback);
 }
 
 void DriveFileSyncService::DidGetLargestChangeStampForBatchSync(
     const SyncStatusCallback& callback,
     const GURL& origin,
     const std::string& resource_id,
     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;
   }
 
   if (metadata_store_->incremental_sync_origins().empty()) {
     largest_fetched_changestamp_ = largest_changestamp;
     metadata_store_->SetLargestChangeStamp(
         largest_changestamp,
         base::Bind(&EmptyStatusCallback));
   }
 
   api_util_->ListFiles(
       resource_id,
       base::Bind(&DriveFileSyncService::DidGetDirectoryContentForBatchSync,
                  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;
   }
 
   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;
   if (feed->GetNextFeedURL(&next_feed_url)) {
     api_util_->ContinueListing(
         next_feed_url,
         base::Bind(&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);
 }
 
 void DriveFileSyncService::ApplyLocalChangeInternal(
     scoped_ptr<ApplyLocalChangeParam> param,
     SyncStatusCode status,
     const std::string& origin_resource_id) {
   if (status != SYNC_STATUS_OK) {
     param->callback.Run(status);
@@ skipping 1320 matching lines @@
 void DriveFileSyncService::DidEnsureOriginRoot(
     const GURL& origin,
     const ResourceIdCallback& callback,
     google_apis::GDataErrorCode error,
     const std::string& resource_id) {
   SyncStatusCode status = GDataErrorCodeToSyncStatusCodeWrapper(error);
   if (status == SYNC_STATUS_OK &&
       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);
 }
 
 std::string DriveFileSyncService::sync_root_resource_id() {
   return metadata_store_->sync_root_directory();
 }
 
 }  // namespace sync_file_system