Implement polling part of DriveFileSyncService

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 fa49969163c897d8a49f7d2b343360284b340161..f73a5ff4b399d481752b430483132773ad161a58 100644
--- a/chrome/browser/sync_file_system/drive_file_sync_service.cc
+++ b/chrome/browser/sync_file_system/drive_file_sync_service.cc
@@ -21,8 +21,9 @@
 #include "chrome/browser/sync_file_system/drive_metadata_store.h"
 #include "chrome/browser/sync_file_system/remote_change_processor.h"
 #include "chrome/browser/sync_file_system/sync_file_system.pb.h"
+#include "chrome/common/extensions/extension.h"
 #include "content/public/browser/browser_thread.h"
-#include "net/base/escape.h"
+#include "extensions/common/constants.h"
 #include "webkit/fileapi/file_system_url.h"
 #include "webkit/fileapi/syncable/sync_file_metadata.h"
 #include "webkit/fileapi/syncable/sync_file_type.h"
@@ -35,6 +36,9 @@ namespace {
 const FilePath::CharType kTempDirName[] = FILE_PATH_LITERAL("tmp");
 const FilePath::CharType kSyncFileSystemDir[] =
     FILE_PATH_LITERAL("Sync FileSystem");
+const int64 kMinimumPollingDelaySeconds = 10;
+const int64 kMaximumPollingDelaySeconds = 60 * 60;  // 1 hour
+const int64 kDelayMultiplier = 2;

[kinuko, 2012/12/03 13:10:34]

nit: if it's int we'll have 1, 2 or etc...? maybe it might make more sense if we
make it double?

[tzik, 2012/12/04 04:59:50]

Done.

 
 bool CreateTemporaryFile(const FilePath& dir_path, FilePath* temp_file) {
   return file_util::CreateDirectory(dir_path) &&
@@ -202,7 +206,9 @@ bool DriveFileSyncService::RemoteChangeComparator::operator()(
 DriveFileSyncService::DriveFileSyncService(Profile* profile)
     : last_operation_status_(fileapi::SYNC_STATUS_OK),
       state_(REMOTE_SERVICE_OK),
-      largest_changestamp_(0),
+      largest_fetched_changestamp_(0),
+      polling_delay_(base::TimeDelta::FromSeconds(kMinimumPollingDelaySeconds)),
+      polling_enabled_(true),
       weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   temporary_file_dir_ =
       profile->GetPath().Append(kSyncFileSystemDir).Append(kTempDirName);
@@ -248,6 +254,7 @@ void DriveFileSyncService::RemoveObserver(Observer* observer) {
 void DriveFileSyncService::RegisterOriginForTrackingChanges(
     const GURL& origin,
     const fileapi::SyncStatusCallback& callback) {
+  DCHECK(origin.SchemeIs(extensions::kExtensionScheme));
   scoped_ptr<TaskToken> token(GetToken(
           FROM_HERE, TASK_TYPE_DRIVE, "Retrieving origin metadata"));
   if (!token) {
@@ -423,7 +430,7 @@ void DriveFileSyncService::ApplyLocalChange(
       sync_client_->UploadNewFile(
           metadata_store_->GetResourceIdForOrigin(url.origin()),
           local_file_path,
-          net::EscapePath(url.path().AsUTF8Unsafe()),
+          url.path().AsUTF8Unsafe(),
           local_file_metadata.size,
           base::Bind(&DriveFileSyncService::DidUploadNewFile,
                      AsWeakPtr(), base::Passed(&token), url, callback));
@@ -503,7 +510,8 @@ DriveFileSyncService::DriveFileSyncService(
     scoped_ptr<DriveMetadataStore> metadata_store)
     : last_operation_status_(fileapi::SYNC_STATUS_OK),
       state_(REMOTE_SERVICE_OK),
-      largest_changestamp_(0),
+      largest_fetched_changestamp_(0),
+      polling_enabled_(false),
       weak_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   temporary_file_dir_ = base_dir.Append(kTempDirName);
 
@@ -532,7 +540,6 @@ void DriveFileSyncService::NotifyTaskDone(fileapi::SyncStatusCode status,
   last_operation_status_ = status;
   token_ = token.Pass();
 
-  RemoteServiceState old_state = state_;
   if (token_->task_type() != TASK_TYPE_NONE) {
     DVLOG(1) << "NotifyTaskDone: " << token_->description()
              << ": finished with status=" << status
@@ -557,7 +564,7 @@ void DriveFileSyncService::NotifyTaskDone(fileapi::SyncStatusCode status,
     return;
   }
 
-  if (state_ != REMOTE_SERVICE_OK || old_state == state_)
+  if (state_ != REMOTE_SERVICE_OK)
     return;
 
   // If the state has become OK and we have any pending batch sync origins
@@ -567,7 +574,15 @@ void DriveFileSyncService::NotifyTaskDone(fileapi::SyncStatusCode status,
     pending_batch_sync_origins_.erase(pending_batch_sync_origins_.begin());
     std::string resource_id = metadata_store_->GetResourceIdForOrigin(origin);
     StartBatchSyncForOrigin(origin, resource_id);
+    return;
   }
+
+  // Notify observer of the right timing to run ProcessRemoteChange.
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    OnRemoteChangeAvailable(pending_changes_.size()));
+
+  if (pending_changes_.empty() && polling_enabled_)
+    SchedulePolling();

[kinuko, 2012/12/03 13:10:34]

I have a feeling that it might be ok to occasionally run this not only if we're
in REMOTE_SERVICE_OK but also in REMOTE_SERVICE_TEMPORARY_UNAVAILABLE?

[kinuko, 2012/12/03 13:10:34]

in OnAuthenticated or OnNetworkConnected should we also call this (maybe after
resetting the polling_delay_ to min)?

[tzik, 2012/12/04 04:59:50]

Done.

[tzik, 2012/12/04 04:59:50]

those patches are not landed yet.
let me do it later.

 }
 
 void DriveFileSyncService::UpdateServiceState() {
@@ -629,6 +644,8 @@ void DriveFileSyncService::DidInitializeMetadataStore(
     return;
   }
 
+  largest_fetched_changestamp_ = metadata_store_->GetLargestChangeStamp();
+
   if (metadata_store_->sync_root_directory().empty()) {
     GetSyncRootDirectory(token.Pass(), base::Bind(&EmptyStatusCallback));
     return;
@@ -739,6 +756,13 @@ void DriveFileSyncService::DidGetLargestChangeStampForBatchSync(
     return;
   }
 
+  if (metadata_store_->incremental_sync_origins().empty()) {
+    largest_fetched_changestamp_ = largest_changestamp;
+    metadata_store_->SetLargestChangeStamp(
+        largest_changestamp,
+        base::Bind(&EmptyStatusCallback));
+  }
+
   DCHECK(token);
   token->UpdateTask(FROM_HERE, TASK_TYPE_DRIVE, "Retrieving remote files");
   sync_client_->ListFiles(
@@ -764,7 +788,7 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
   typedef ScopedVector<google_apis::DocumentEntry>::const_iterator iterator;
   for (iterator itr = feed->entries().begin();
        itr != feed->entries().end(); ++itr) {
-    AppendNewRemoteChange(origin, *itr, largest_changestamp,
+    AppendNewRemoteChange(origin, **itr, largest_changestamp,
                           REMOTE_SYNC_TYPE_BATCH);
   }
 
@@ -869,7 +893,7 @@ void DriveFileSyncService::DidApplyLocalChange(
     const fileapi::SyncStatusCallback& callback,
     fileapi::SyncStatusCode status) {
   if (status == fileapi::SYNC_STATUS_OK) {
-    CancelRemoteChange(url);
+    RemoveRemoteChange(url);
     NotifyTaskDone(GDataErrorCodeToSyncStatusCodeWrapper(error), token.Pass());
     callback.Run(GDataErrorCodeToSyncStatusCodeWrapper(error));
     return;
@@ -1004,6 +1028,7 @@ void DriveFileSyncService::DidPrepareForProcessRemoteChange(
   bool missing_db_entry = (status != fileapi::SYNC_STATUS_OK);
   if (missing_db_entry) {
     param->drive_metadata.set_resource_id(param->remote_change.resource_id);
+    param->drive_metadata.set_md5_checksum(std::string());
     param->drive_metadata.set_conflicted(false);
   }
   bool missing_local_file =
@@ -1083,8 +1108,10 @@ void DriveFileSyncService::DidPrepareForProcessRemoteChange(
     }
     DCHECK(!missing_local_file);
     param->operation_type = fileapi::SYNC_OPERATION_DELETE;
+
+    const fileapi::FileChange& file_change = param->remote_change.change;
     param->processor->ApplyRemoteChange(
-        param->remote_change.change, FilePath(), url,
+        file_change, FilePath(), url,
         base::Bind(&DriveFileSyncService::DidApplyRemoteChange, AsWeakPtr(),
                    base::Passed(&param)));
     return;
@@ -1123,13 +1150,11 @@ void DriveFileSyncService::DidGetTemporaryFileForDownload(
     return;
   }
 
-  DriveMetadata metadata;
-  metadata_store_->ReadEntry(param->remote_change.url, &metadata);
-
   const FilePath& temporary_file_path = param->temporary_file_path;
   std::string resource_id = param->remote_change.resource_id;
+  const DriveMetadata& drive_metadata = param->drive_metadata;
   sync_client_->DownloadFile(
-      resource_id, metadata.md5_checksum(),
+      resource_id, drive_metadata.md5_checksum(),
       temporary_file_path,
       base::Bind(&DriveFileSyncService::DidDownloadFile,
                  AsWeakPtr(), base::Passed(&param)));
@@ -1151,7 +1176,7 @@ void DriveFileSyncService::DidDownloadFile(
     return;
   }
 
-  param->md5_checksum = md5_checksum;
+  param->drive_metadata.set_md5_checksum(md5_checksum);
   const fileapi::FileChange& change = param->remote_change.change;
   const FilePath& temporary_file_path = param->temporary_file_path;
   const fileapi::FileSystemURL& url = param->remote_change.url;
@@ -1175,13 +1200,12 @@ void DriveFileSyncService::DidApplyRemoteChange(
     return;
   }
 
-  DriveMetadata metadata;
-  metadata.set_resource_id(param->remote_change.resource_id);
-  metadata.set_md5_checksum(param->md5_checksum);
-  metadata.set_conflicted(false);
+  const DriveMetadata& drive_metadata = param->drive_metadata;
+  param->drive_metadata.set_resource_id(param->remote_change.resource_id);
+  param->drive_metadata.set_conflicted(false);
 
   metadata_store_->UpdateEntry(
-      url, metadata,
+      url, drive_metadata,
       base::Bind(&DriveFileSyncService::CompleteRemoteSync,
                  AsWeakPtr(), base::Passed(&param)));
 }
@@ -1203,7 +1227,7 @@ void DriveFileSyncService::CompleteRemoteSync(
     return;
   }
 
-  CancelRemoteChange(param->remote_change.url);
+  RemoveRemoteChange(param->remote_change.url);
 
   GURL origin = param->remote_change.url.origin();
   if (metadata_store_->IsIncrementalSyncOrigin(origin)) {
@@ -1244,11 +1268,11 @@ void DriveFileSyncService::FinalizeRemoteSync(
 
 void DriveFileSyncService::AppendNewRemoteChange(
     const GURL& origin,
-    google_apis::DocumentEntry* entry,
+    const google_apis::DocumentEntry& entry,
     int64 changestamp,
     RemoteSyncType sync_type) {
   // TODO(tzik): Normalize the path here.
-  FilePath path = FilePath::FromUTF8Unsafe(UTF16ToUTF8(entry->title()));
+  FilePath path = FilePath::FromUTF8Unsafe(UTF16ToUTF8(entry.title()));
 
   PathToChange* path_to_change = &url_to_change_[origin];
   PathToChange::iterator found = path_to_change->find(path);
@@ -1263,12 +1287,12 @@ void DriveFileSyncService::AppendNewRemoteChange(
 
   fileapi::FileChange::ChangeType change_type;
   fileapi::SyncFileType file_type;
-  if (entry->deleted()) {
+  if (entry.deleted()) {
     change_type = fileapi::FileChange::FILE_CHANGE_DELETE;
     file_type = fileapi::SYNC_FILE_TYPE_UNKNOWN;
   } else {
     change_type = fileapi::FileChange::FILE_CHANGE_ADD_OR_UPDATE;
-    if (entry->kind() == google_apis::ENTRY_KIND_FOLDER)
+    if (entry.is_folder())
       file_type = fileapi::SYNC_FILE_TYPE_DIRECTORY;
     else
       file_type = fileapi::SYNC_FILE_TYPE_FILE;
@@ -1281,11 +1305,11 @@ void DriveFileSyncService::AppendNewRemoteChange(
   DCHECK(inserted_to_queue.second);
 
   (*path_to_change)[path] = RemoteChange(
-      changestamp, entry->resource_id(), url, file_change,
+      changestamp, entry.resource_id(), url, file_change,
       inserted_to_queue.first);
 }
 
-void DriveFileSyncService::CancelRemoteChange(
+void DriveFileSyncService::RemoveRemoteChange(
     const fileapi::FileSystemURL& url) {
   URLToChange::iterator found_origin = url_to_change_.find(url.origin());
   if (found_origin == url_to_change_.end())
@@ -1301,14 +1325,10 @@ void DriveFileSyncService::CancelRemoteChange(
   if (path_to_change->empty())
     url_to_change_.erase(found_origin);
 
-  MaybeMarkAsIncrementalSyncOrigin(url.origin());
-}
-
-void DriveFileSyncService::MaybeMarkAsIncrementalSyncOrigin(
-    const GURL& origin) {
-  if (metadata_store_->IsBatchSyncOrigin(origin) &&
-      !ContainsKey(url_to_change_, origin))
-    metadata_store_->MoveBatchSyncOriginToIncremental(origin);
+  if (metadata_store_->IsBatchSyncOrigin(url.origin()) &&
+      !ContainsKey(url_to_change_, url.origin())) {
+    metadata_store_->MoveBatchSyncOriginToIncremental(url.origin());

[kinuko, 2012/12/03 13:10:34]

Shouldn't we call SchedulePolling() but not kick the timer in SchedulePolling()
if incremental_sync_origins_.empty()?

+  }
 }
 
 bool DriveFileSyncService::GetPendingChangeForFileSystemURL(
@@ -1326,6 +1346,114 @@ bool DriveFileSyncService::GetPendingChangeForFileSystemURL(
   return true;
 }
 
+void DriveFileSyncService::FetchChangesForIncrementalSync() {
+  scoped_ptr<TaskToken> token(GetToken(FROM_HERE, TASK_TYPE_DRIVE,
+                                       "Fetching remote change list"));
+  if (!token) {
+    pending_tasks_.push_back(base::Bind(
+        &DriveFileSyncService::FetchChangesForIncrementalSync, AsWeakPtr()));
+    return;
+  }
+
+  if (metadata_store_->incremental_sync_origins().empty()) {
+    NotifyTaskDone(fileapi::SYNC_STATUS_OK, token.Pass());
+    return;
+  }

[kinuko, 2012/12/03 13:10:34]

Don't we call FetchChangesForIncrementalSync() repeatedly with minimum delay
while we have no incremental_sync_origins in this way?

[tzik, 2012/12/04 04:59:50]

Done.

+
+  sync_client_->ListChanges(
+      largest_fetched_changestamp_,
+      base::Bind(&DriveFileSyncService::DidFetchChangesForIncrementalSync,
+                 AsWeakPtr(), base::Passed(&token)));
+}
+
+void DriveFileSyncService::DidFetchChangesForIncrementalSync(
+    scoped_ptr<TaskToken> token,
+    google_apis::GDataErrorCode error,
+    scoped_ptr<google_apis::DocumentFeed> changes) {
+  if (error != google_apis::HTTP_SUCCESS) {
+    NotifyTaskDone(fileapi::SYNC_STATUS_FAILED, token.Pass());
+    return;
+  }
+
+  GURL next_feed;

[kinuko, 2012/12/03 13:10:34]

nit: can you move this right before line 1392?

[tzik, 2012/12/04 04:59:50]

Done.

+
+  typedef ScopedVector<google_apis::DocumentEntry>::const_iterator iterator;
+  for (iterator itr = changes->entries().begin();
+       itr != changes->entries().end(); ++itr) {
+    const google_apis::DocumentEntry& entry = **itr;
+    GURL origin;
+    if (!GetOriginForEntry(entry, &origin))
+      continue;
+
+    AppendNewRemoteChange(origin, entry, entry.changestamp(),
+                          REMOTE_SYNC_TYPE_INCREMENTAL);
+  }
+
+  if (changes->GetNextFeedURL(&next_feed)) {
+    sync_client_->ContinueListing(
+        next_feed,
+        base::Bind(&DriveFileSyncService::DidFetchChangesForIncrementalSync,
+                   AsWeakPtr(), base::Passed(&token)));

[kinuko, 2012/12/03 13:10:34]

nit: can we return here and remove else on line 1397?

[tzik, 2012/12/04 04:59:50]

Done.

+  } else {
+    largest_fetched_changestamp_ = changes->largest_changestamp();
+
+    if (changes->start_index() == 0 && changes->entries().empty()) {
+      // If this set of changes is the first feed and its empty, update

[kinuko, 2012/12/03 13:10:34]

nit: its empty -> it's empty

[tzik, 2012/12/04 04:59:50]

Done.

+      // the polling delay to wait longer.
+      base::TimeDelta max_delay(
+          base::TimeDelta::FromSeconds(kMaximumPollingDelaySeconds));
+
+      polling_delay_ *= kDelayMultiplier;
+      if (polling_delay_ >= max_delay)
+        polling_delay_ = max_delay;
+    } else {
+      polling_delay_ = base::TimeDelta::FromSeconds(
+          kMinimumPollingDelaySeconds);
+    }
+
+    NotifyTaskDone(fileapi::SYNC_STATUS_OK, token.Pass());
+  }
+}
+
+bool DriveFileSyncService::GetOriginForEntry(
+    const google_apis::DocumentEntry& entry,
+    GURL* origin_out) {
+  typedef ScopedVector<google_apis::Link>::const_iterator iterator;
+  for (iterator itr = entry.links().begin();
+       itr != entry.links().end(); ++itr) {
+    if ((*itr)->type() != google_apis::Link::LINK_PARENT)
+      continue;
+    GURL origin(UTF16ToUTF8((*itr)->title()));
+    if (!origin.is_valid())
+      continue;
+
+    if (!metadata_store_->IsBatchSyncOrigin(origin) &&
+        !metadata_store_->IsIncrementalSyncOrigin(origin))
+      continue;
+    std::string resource_id(metadata_store_->GetResourceIdForOrigin(origin));
+    GURL resource_link(sync_client_->ResourceIdToResourceLink(resource_id));
+    if ((*itr)->href().GetOrigin() != resource_link.GetOrigin() ||
+        (*itr)->href().path() != resource_link.path())
+      continue;
+
+    *origin_out = origin;
+    return true;
+  }
+  return false;
+}
+
+void DriveFileSyncService::SchedulePolling() {
+  if (!polling_timer_.IsRunning()) {

[kinuko, 2012/12/03 13:10:34]

nit: maybe we can early return?

if (polling_timer_.IsRunning())
  return;

...

[tzik, 2012/12/04 04:59:50]

Done.

+    DVLOG(1) << "Polling scheduled"
+             << " (delay:" << polling_delay_.InSeconds() << ")";
+
+    polling_timer_.Start(
+        FROM_HERE, polling_delay_,
+        base::Bind(&DriveFileSyncService::FetchChangesForIncrementalSync,
+                   AsWeakPtr()));
+  }
+}
+
 fileapi::SyncStatusCode
 DriveFileSyncService::GDataErrorCodeToSyncStatusCodeWrapper(
     google_apis::GDataErrorCode error) const {