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 362 matching lines @@
 
 void DriveFileSyncService::DidInitializeMetadataStore(
     const SyncStatusCallback& callback,
     SyncStatusCode status,
     bool created) {
   if (status != SYNC_STATUS_OK) {
     callback.Run(status);
     return;
   }
 
+  // Guarantee that metadata_store_->batch_sync_origins is not used.
+  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();
 
   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);
-  }
-
   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;
+    if (!metadata_store_->IsIncrementalSyncOrigin(url.origin())) {
+      metadata_store_->DeleteEntry(url, base::Bind(&EmptyStatusCallback));
+      continue;
+    }
     AppendFetchChange(url.origin(), url.path(), resource_id, file_type);
   }
 
   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 =
@@ skipping 82 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) &&

[kinuko, 2013/05/20 15:31:50]

This line could be removed too?

[calvinlo, 2013/05/21 07:12:26]

Done.

       !metadata_store_->IsIncrementalSyncOrigin(origin)) {
     callback.Run(SYNC_STATUS_OK);
     return;
   }
 
   remote_change_handler_.RemoveChangesForOrigin(origin);
   pending_batch_sync_origins_.erase(origin);
   metadata_store_->DisableOrigin(origin, callback);
 }
 
 void DriveFileSyncService::DoUninstallOrigin(
     const GURL& origin,
     const SyncStatusCallback& callback) {
+  // Because origin management is now split between DriveFileSyncService and
+  // DriveMetadataStore, it must be checked for in two places.
   std::string resource_id = metadata_store_->GetResourceIdForOrigin(origin);
+  if (resource_id.empty()) {
+    std::map<GURL, std::string>::const_iterator iterator =
+        pending_batch_sync_origins_.find(origin);
+    if (iterator != pending_batch_sync_origins_.end())
+      resource_id = iterator->second;
+  }
 
   // An empty resource_id indicates either one of following two cases:
   // 1) origin is not in metadata_store_ because the extension was never
   //    run or it's not managed by this service, and thus no
   //    origin directory on the remote drive was created.
   // 2) origin or sync root folder is deleted on Drive.
   if (resource_id.empty()) {
     callback.Run(SYNC_STATUS_UNKNOWN_ORIGIN);
     return;
   }
@@ skipping 70 matching lines @@
   running_local_sync_task_->Run(base::Bind(
       &DriveFileSyncService::DidApplyLocalChange, AsWeakPtr(), callback));
 }
 
 void DriveFileSyncService::UpdateRegisteredOrigins() {
   ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   if (!extension_service)
     return;
 
+  // Must combine batch sync origins with incremental and disabled origins since
+  // batch origins are tracked in DriveFileSyncService but the others are in
+  // DriveMetaDataStore.
   std::vector<GURL> origins;
   metadata_store_->GetAllOrigins(&origins);
+  typedef std::map<GURL, std::string>::const_iterator iterator;
+  for (iterator itr = pending_batch_sync_origins_.begin();
+       itr != pending_batch_sync_origins_.end(); ++itr) {
+    DCHECK(!metadata_store_->IsKnownOrigin(itr->first));
+    origins.push_back(itr->first);
+  }

[kinuko, 2013/05/20 15:31:50]

We seem to be checking pending_batch_sync_origins_ is empty right before we call
this method?

I guessed this might be added for testing, but if so probably we should change
the test code?

[calvinlo, 2013/05/21 07:12:26]

Thanks for catching this. I have removed the errorneous code. I have also
adjusted the tests conform to the new behavior.

+
+  // Update the status of every origin using status from ExtensionService.
   for (std::vector<GURL>::const_iterator itr = origins.begin();
        itr != origins.end(); ++itr) {
     std::string extension_id = itr->host();
     GURL origin =
         extensions::Extension::GetBaseURLFromExtensionId(extension_id);
+    bool is_pending_batch_sync_origin = (pending_batch_sync_origins_.find(
+        origin) != pending_batch_sync_origins_.end());
 
     if (!extension_service->GetInstalledExtension(extension_id)) {
       // Extension has been uninstalled.
+      pending_batch_sync_origins_.erase(origin);
       UninstallOrigin(origin, base::Bind(&EmptyStatusCallback));
-    } else if ((metadata_store_->IsBatchSyncOrigin(origin) ||
-                metadata_store_->IsIncrementalSyncOrigin(origin)) &&
+    } else if ((is_pending_batch_sync_origin ||
+                   metadata_store_->IsBatchSyncOrigin(origin)) &&

[kinuko, 2013/05/20 15:31:50]

IsBatchSyncOrigin(origin) is always false, right?

[calvinlo, 2013/05/21 07:12:26]

I have added DCHECKS at the beginning to ensure this.

                !extension_service->IsExtensionEnabled(extension_id)) {
-      // Extension has been disabled.
+      // pending batch sync origin has been disabled
+      pending_batch_sync_origins_.erase(origin);
+      UninstallOrigin(origin, base::Bind(&EmptyStatusCallback));
+    } else if (metadata_store_->IsIncrementalSyncOrigin(origin) &&
+               !extension_service->IsExtensionEnabled(extension_id)) {
+      // Incremental 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));
   }

[kinuko, 2013/05/20 15:31:50]

nit: no { } for one-line body

[calvinlo, 2013/05/21 07:12:26]

Done.

 
   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));
+    // TODO(calvinlo): Write metadata and origin data as single batch command
+    // so it's not possible for the DB to contain a DriveMetadata with an
+    // unknown origin.
+    metadata_store_->UpdateEntry(url, metadata,
+                                 base::Bind(&EmptyStatusCallback));
   }
 
   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);
 }
 
 // TODO(tzik): Factor out this conflict resolution function.
 ConflictResolutionResult DriveFileSyncService::ResolveConflictForLocalSync(
     SyncFileType local_file_type,
     const base::Time& local_updated_time,
     SyncFileType remote_file_type,
     const base::Time& remote_updated_time) {
@@ skipping 567 matching lines @@
   return true;
 }
 
 void DriveFileSyncService::RemoveRemoteChange(
     const FileSystemURL& url) {
   if (!remote_change_handler_.RemoveChangeForURL(url))
     return;
   if (metadata_store_->IsBatchSyncOrigin(url.origin()) &&
       !remote_change_handler_.HasChangesForOrigin(url.origin())) {
     metadata_store_->MoveBatchSyncOriginToIncremental(url.origin());
   }

[kinuko, 2013/05/20 15:31:50]

These lines could be removed too?

[calvinlo, 2013/05/21 07:12:26]

Done. I think it's ok to delete these because we move origins directly to
incremental sync mode now.

 }
 
 void DriveFileSyncService::MarkConflict(
     const fileapi::FileSystemURL& url,
     DriveMetadata* drive_metadata,
     const SyncStatusCallback& callback) {
   DCHECK(drive_metadata);
   if (drive_metadata->resource_id().empty()) {
     // If the file does not have valid drive_metadata in the metadata store
     // we must have a pending remote change entry.
@@ skipping 317 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 15:31:50]

These 2 lines are not needed anymore?

[calvinlo, 2013/05/21 07:12:26]

Done.

   }
   callback.Run(status, resource_id);
 }
 
 std::string DriveFileSyncService::sync_root_resource_id() {
   return metadata_store_->sync_root_directory();
 }
 
 }  // namespace sync_file_system