Clear syncing flag after a remote or local sync

- Stop clearing sync flag locally in ProcessLocalChange for local sync
- *Always* clear sync flag after a local or remote sync

BUG=155505
TEST=LocalFileSyncServiceTest.*

Review URL: https://codereview.chromium.org/11411352

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170948 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync_file_system/local_file_sync_service.cc

@@ -148,6 +148,12 @@ void LocalFileSyncService::HasPendingLocalChanges(
       origin_to_contexts_[url.origin()], url, callback);
 }
 
+void LocalFileSyncService::ClearSyncFlagForURL(
+    const fileapi::FileSystemURL& url) {
+  DCHECK(ContainsKey(origin_to_contexts_, url.origin()));
+  sync_context_->ClearSyncFlagForURL(url);
+}
+
 void LocalFileSyncService::GetLocalFileMetadata(
     const fileapi::FileSystemURL& url,
     const SyncFileMetadataCallback& callback) {
@@ -217,7 +223,7 @@ void LocalFileSyncService::DidGetFileForLocalSync(
     const LocalFileSyncInfo& sync_file_info) {
   DCHECK(!local_sync_callback_.is_null());
   if (status != fileapi::SYNC_STATUS_OK) {
-    RunLocalSyncCallback(status, FileSystemURL());
+    RunLocalSyncCallback(status, sync_file_info.url);
     return;
   }
   if (sync_file_info.changes.empty()) {
@@ -272,7 +278,11 @@ void LocalFileSyncService::ProcessNextChangeForURL(
       // OR has failed due to conflict (so that we won't stick to the same
       // conflicting file again and again).
       DCHECK(ContainsKey(origin_to_contexts_, url.origin()));
-      sync_context_->FinalizeSyncForURL(origin_to_contexts_[url.origin()], url);
+      sync_context_->ClearChangesForURL(
+          origin_to_contexts_[url.origin()], url,
+          base::Bind(&LocalFileSyncService::RunLocalSyncCallback,
+                     AsWeakPtr(), status, url));
+      return;
     }
     RunLocalSyncCallback(status, url);
     return;

chrome/browser/sync_file_system/local_file_sync_service.h

@@ -89,6 +89,10 @@ class LocalFileSyncService
       const fileapi::FileSystemURL& url,
       const HasPendingLocalChangeCallback& callback);
 
+  // A local or remote sync has been finished (either successfully or
+  // with an error). Clears the internal sync flag and enable writing for |url|.
+  void ClearSyncFlagForURL(const fileapi::FileSystemURL& url);
+
   // Returns the metadata of a remote file pointed by |url|.
   virtual void GetLocalFileMetadata(
       const fileapi::FileSystemURL& url,

chrome/browser/sync_file_system/local_file_sync_service_unittest.cc

@@ -314,13 +314,10 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_CreateFile) {
 
   base::RunLoop run_loop;
 
-  // We should get called OnSyncEnabled and OnWriteEnabled on kFile.
-  // (We quit the run loop when OnWriteEnabled is called on kFile)
+  // We should get called OnSyncEnabled on kFile.
   StrictMock<MockSyncStatusObserver> status_observer;
   EXPECT_CALL(status_observer, OnSyncEnabled(kFile))
       .Times(AtLeast(1));
-  EXPECT_CALL(status_observer, OnWriteEnabled(kFile))
-      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
   file_system_->AddSyncStatusObserver(&status_observer);
 
   // Creates and writes into a file.
@@ -353,7 +350,7 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_CreateFile) {
 
   local_service_->ProcessLocalChange(
       &local_change_processor,
-      base::Bind(&OnSyncCompleted, FROM_HERE, base::Bind(&base::DoNothing),
+      base::Bind(&OnSyncCompleted, FROM_HERE, run_loop.QuitClosure(),
                  fileapi::SYNC_STATUS_OK, kFile));
 
   run_loop.Run();
@@ -369,12 +366,9 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_CreateAndRemoveFile) {
   base::RunLoop run_loop;
 
   // We should get called OnSyncEnabled and OnWriteEnabled on kFile.
-  // (We quit the run loop when OnWriteEnabled is called on kFile)
   StrictMock<MockSyncStatusObserver> status_observer;
   EXPECT_CALL(status_observer, OnSyncEnabled(kFile))
       .Times(AtLeast(1));
-  EXPECT_CALL(status_observer, OnWriteEnabled(kFile))
-      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
   file_system_->AddSyncStatusObserver(&status_observer);
 
   // Creates and then deletes a file.
@@ -394,7 +388,7 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_CreateAndRemoveFile) {
   // The sync should succeed anyway.
   local_service_->ProcessLocalChange(
       &local_change_processor,
-      base::Bind(&OnSyncCompleted, FROM_HERE, base::Bind(&base::DoNothing),
+      base::Bind(&OnSyncCompleted, FROM_HERE, run_loop.QuitClosure(),
                  fileapi::SYNC_STATUS_OK, kFile));
 
   run_loop.Run();
@@ -409,8 +403,7 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_CreateAndRemoveDirectory) {
 
   base::RunLoop run_loop;
 
-  // OnSyncEnabled is expected to be called at least or more than once
-  // but OnWriteEnabled will never be called.
+  // OnSyncEnabled is expected to be called at least or more than once.
   StrictMock<MockSyncStatusObserver> status_observer;
   EXPECT_CALL(status_observer, OnSyncEnabled(kDir)).Times(AtLeast(1));
   file_system_->AddSyncStatusObserver(&status_observer);
@@ -442,12 +435,9 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_MultipleChanges) {
 
   // We should get called OnSyncEnabled and OnWriteEnabled on kPath and
   // OnSyncEnabled on kOther.
-  // (We quit the run loop when OnWriteEnabled is called on kPath)
   StrictMock<MockSyncStatusObserver> status_observer;
   EXPECT_CALL(status_observer, OnSyncEnabled(kPath)).Times(AtLeast(1));
   EXPECT_CALL(status_observer, OnSyncEnabled(kOther)).Times(AtLeast(1));
-  EXPECT_CALL(status_observer, OnWriteEnabled(kPath))
-      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::Quit));
   file_system_->AddSyncStatusObserver(&status_observer);
 
   // Creates a file, delete the file and creates a directory with the same
@@ -472,7 +462,7 @@ TEST_F(LocalFileSyncServiceTest, ProcessLocalChange_MultipleChanges) {
 
   local_service_->ProcessLocalChange(
       &local_change_processor,
-      base::Bind(&OnSyncCompleted, FROM_HERE, base::Bind(&base::DoNothing),
+      base::Bind(&OnSyncCompleted, FROM_HERE, run_loop.QuitClosure(),
                  fileapi::SYNC_STATUS_OK, kPath));
 
   run_loop.Run();

chrome/browser/sync_file_system/sync_file_system_service.cc

@@ -382,6 +382,13 @@ void SyncFileSystemService::DidProcessRemoteChange(
            << " operation_type=" << type;
   DCHECK(remote_sync_running_);
   remote_sync_running_ = false;
+
+  if (status != fileapi::SYNC_STATUS_NO_CHANGE_TO_SYNC &&
+      remote_file_service_->GetCurrentState() != REMOTE_SERVICE_DISABLED) {
+    DCHECK(url.is_valid());
+    local_file_service_->ClearSyncFlagForURL(url);
+  }
+
   if (status == fileapi::SYNC_STATUS_OK &&
       type != fileapi::SYNC_OPERATION_NONE) {
     // Notify observers of the changes made for a remote sync.
@@ -411,6 +418,12 @@ void SyncFileSystemService::DidProcessLocalChange(
            << " url=" << url.DebugString();
   DCHECK(local_sync_running_);
   local_sync_running_ = false;
+
+  if (status != fileapi::SYNC_STATUS_NO_CHANGE_TO_SYNC) {
+    DCHECK(url.is_valid());
+    local_file_service_->ClearSyncFlagForURL(url);
+  }
+
   if (status == fileapi::SYNC_STATUS_NO_CHANGE_TO_SYNC) {
     // We seem to have no changes to work on for now.
     // TODO(kinuko): Might be better setting a timer to call MaybeStartSync.

webkit/fileapi/syncable/local_file_sync_context.cc

@@ -92,9 +92,10 @@ void LocalFileSyncContext::GetFileForLocalSync(
                  base::Owned(urls), callback));
 }
 
-void LocalFileSyncContext::FinalizeSyncForURL(
+void LocalFileSyncContext::ClearChangesForURL(
     FileSystemContext* file_system_context,
-    const FileSystemURL& url) {
+    const FileSystemURL& url,
+    const base::Closure& done_callback) {
   // This is initially called on UI thread and to be relayed to FILE thread.
   DCHECK(file_system_context);
   if (!file_system_context->task_runners()->file_task_runner()->
@@ -102,14 +103,20 @@ void LocalFileSyncContext::FinalizeSyncForURL(
     DCHECK(ui_task_runner_->RunsTasksOnCurrentThread());
     file_system_context->task_runners()->file_task_runner()->PostTask(
         FROM_HERE,
-        base::Bind(&LocalFileSyncContext::FinalizeSyncForURL,
-                   this, make_scoped_refptr(file_system_context), url));
+        base::Bind(&LocalFileSyncContext::ClearChangesForURL,
+                   this, make_scoped_refptr(file_system_context),
+                   url, done_callback));
     return;
   }
   DCHECK(file_system_context->change_tracker());
   file_system_context->change_tracker()->ClearChangesForURL(url);
 
-  // Call out to the IO thread to re-enable writing.
+  // Call the completion callback on UI thread.
+  ui_task_runner_->PostTask(FROM_HERE, done_callback);
+}
+
+void LocalFileSyncContext::ClearSyncFlagForURL(const FileSystemURL& url) {
+  // This is initially called on UI thread and to be relayed to IO thread.
   io_task_runner_->PostTask(
       FROM_HERE,
       base::Bind(&LocalFileSyncContext::EnableWritingOnIOThread,

webkit/fileapi/syncable/local_file_sync_context.h

@@ -76,11 +76,17 @@ class WEBKIT_STORAGE_EXPORT LocalFileSyncContext
   void GetFileForLocalSync(FileSystemContext* file_system_context,
                            const LocalFileSyncInfoCallback& callback);
 
-  // Notifies the sync context that the local sync has finished (either
-  // successfully or with an error) for |url|.
+  // Clears all pending local changes for |url|. |done_callback| is called
+  // when the changes are cleared.
   // This method must be called on UI thread.
-  void FinalizeSyncForURL(FileSystemContext* file_system_context,
-                          const FileSystemURL& url);
+  void ClearChangesForURL(FileSystemContext* file_system_context,
+                          const FileSystemURL& url,
+                          const base::Closure& done_callback);
+
+  // A local or remote sync has been finished (either successfully or
+  // with an error). Clears the internal sync flag and enable writing for |url|.
+  // This method must be called on UI thread.
+  void ClearSyncFlagForURL(const FileSystemURL& url);
 
   // Prepares for sync |url| by disabling writes on |url|.
   // If the target |url| is being written and cannot start sync it
@@ -208,7 +214,7 @@ class WEBKIT_STORAGE_EXPORT LocalFileSyncContext
       const FileSystemURL& url,
       const LocalFileSyncInfoCallback& callback);
 
-  // Helper routine for FinalizeSyncForURL.
+  // Helper routine for ClearSyncFlagForURL.
   void EnableWritingOnIOThread(const FileSystemURL& url);
 
   // Callback routine for ApplyRemoteChange.