Wait for parent directory sync before performing server-side copy
Add SyncClient::WaitForUpdateTaskToComplete().
Use the new method from CoypOperation.
BUG=384213
TEST=unit_tests
Review URL: https://codereview.chromium.org/372713004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284877 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/chromeos/drive/file_system.cc b/chrome/browser/chromeos/drive/file_system.cc
index 3665383..1669177e 100644
--- a/chrome/browser/chromeos/drive/file_system.cc
+++ b/chrome/browser/chromeos/drive/file_system.cc
@@ -827,6 +827,11 @@
OnDriveSyncError(type, *file_path));
}
+bool FileSystem::WaitForSyncComplete(const std::string& local_id,
+ const FileOperationCallback& callback) {
+ return sync_client_->WaitForUpdateTaskToComplete(local_id, callback);
+}
+
void FileSystem::OnDirectoryReloaded(const base::FilePath& directory_path) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
diff --git a/chrome/browser/chromeos/drive/file_system.h b/chrome/browser/chromeos/drive/file_system.h
index f16c7bf..b4e14d6 100644
--- a/chrome/browser/chromeos/drive/file_system.h
+++ b/chrome/browser/chromeos/drive/file_system.h
@@ -168,6 +168,9 @@
virtual void OnEntryUpdatedByOperation(const std::string& local_id) OVERRIDE;
virtual void OnDriveSyncError(file_system::DriveSyncErrorType type,
const std::string& local_id) OVERRIDE;
+ virtual bool WaitForSyncComplete(
+ const std::string& local_id,
+ const FileOperationCallback& callback) OVERRIDE;
// ChangeListLoader::Observer overrides.
// Used to propagate events from ChangeListLoader.
diff --git a/chrome/browser/chromeos/drive/file_system/copy_operation.cc b/chrome/browser/chromeos/drive/file_system/copy_operation.cc
index 0e42f7b..a0055f86 100644
--- a/chrome/browser/chromeos/drive/file_system/copy_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/copy_operation.cc
@@ -351,21 +351,66 @@
return;
}
- base::FilePath new_title = params->dest_file_path.BaseName();
- if (params->src_entry.file_specific_info().is_hosted_document()) {
+ if (params->parent_entry.resource_id().empty()) {
+ // Parent entry may be being synced.
+ const bool waiting = observer_->WaitForSyncComplete(
+ params->parent_entry.local_id(),
+ base::Bind(&CopyOperation::CopyAfterParentSync,
+ weak_ptr_factory_.GetWeakPtr(), *params));
+ if (!waiting)
+ params->callback.Run(FILE_ERROR_NOT_FOUND);
+ } else {
+ CopyAfterGetParentResourceId(*params, ¶ms->parent_entry, FILE_ERROR_OK);
+ }
+}
+
+void CopyOperation::CopyAfterParentSync(const CopyParams& params,
+ FileError error) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ DCHECK(!params.callback.is_null());
+
+ if (error != FILE_ERROR_OK) {
+ params.callback.Run(error);
+ return;
+ }
+
+ ResourceEntry* parent = new ResourceEntry;
+ base::PostTaskAndReplyWithResult(
+ blocking_task_runner_,
+ FROM_HERE,
+ base::Bind(&internal::ResourceMetadata::GetResourceEntryById,
+ base::Unretained(metadata_),
+ params.parent_entry.local_id(), parent),
+ base::Bind(&CopyOperation::CopyAfterGetParentResourceId,
+ weak_ptr_factory_.GetWeakPtr(), params, base::Owned(parent)));
+}
+
+void CopyOperation::CopyAfterGetParentResourceId(const CopyParams& params,
+ const ResourceEntry* parent,
+ FileError error) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+ DCHECK(!params.callback.is_null());
+
+ if (error != FILE_ERROR_OK) {
+ params.callback.Run(error);
+ return;
+ }
+
+ base::FilePath new_title = params.dest_file_path.BaseName();
+ if (params.src_entry.file_specific_info().is_hosted_document()) {
// Drop the document extension, which should not be in the title.
// TODO(yoshiki): Remove this code with crbug.com/223304.
new_title = new_title.RemoveExtension();
}
base::Time last_modified =
- params->preserve_last_modified ?
+ params.preserve_last_modified ?
base::Time::FromInternalValue(
- params->src_entry.file_info().last_modified()) : base::Time();
+ params.src_entry.file_info().last_modified()) : base::Time();
CopyResourceOnServer(
- params->src_entry.resource_id(), params->parent_entry.resource_id(),
- new_title.AsUTF8Unsafe(), last_modified, params->callback);
+ params.src_entry.resource_id(), parent->resource_id(),
+ new_title.AsUTF8Unsafe(), last_modified, params.callback);
}
void CopyOperation::TransferFileFromLocalToRemote(
diff --git a/chrome/browser/chromeos/drive/file_system/copy_operation.h b/chrome/browser/chromeos/drive/file_system/copy_operation.h
index e0c2385..ea9df03 100644
--- a/chrome/browser/chromeos/drive/file_system/copy_operation.h
+++ b/chrome/browser/chromeos/drive/file_system/copy_operation.h
@@ -89,6 +89,14 @@
const bool* should_copy_on_server,
FileError error);
+ // Part of Copy(). Called after the parent entry gets synced.
+ void CopyAfterParentSync(const CopyParams& params, FileError error);
+
+ // Part of Copy(). Called after the parent resource ID is resolved.
+ void CopyAfterGetParentResourceId(const CopyParams& params,
+ const ResourceEntry* parent,
+ FileError error);
+
// Part of TransferFileFromLocalToRemote(). Called after preparation is done.
// |gdoc_resource_id| and |parent_resource_id| is available only if the file
// is JSON GDoc file.
diff --git a/chrome/browser/chromeos/drive/file_system/copy_operation_unittest.cc b/chrome/browser/chromeos/drive/file_system/copy_operation_unittest.cc
index 85e60567..5d4fa1cc 100644
--- a/chrome/browser/chromeos/drive/file_system/copy_operation_unittest.cc
+++ b/chrome/browser/chromeos/drive/file_system/copy_operation_unittest.cc
@@ -10,6 +10,7 @@
#include "chrome/browser/chromeos/drive/file_change.h"
#include "chrome/browser/chromeos/drive/file_system/operation_test_base.h"
#include "chrome/browser/chromeos/drive/file_system_util.h"
+#include "chrome/browser/chromeos/drive/resource_metadata.h"
#include "chrome/browser/drive/drive_api_util.h"
#include "chrome/browser/drive/fake_drive_service.h"
#include "content/public/test/test_utils.h"
@@ -20,6 +21,20 @@
namespace drive {
namespace file_system {
+namespace {
+
+// Used to handle WaitForSyncComplete() calls.
+bool CopyWaitForSyncCompleteArguments(std::string* out_local_id,
+ FileOperationCallback* out_callback,
+ const std::string& local_id,
+ const FileOperationCallback& callback) {
+ *out_local_id = local_id;
+ *out_callback = callback;
+ return true;
+}
+
+} // namespace
+
class CopyOperationTest : public OperationTestBase {
protected:
virtual void SetUp() OVERRIDE {
@@ -426,5 +441,83 @@
entry2.file_info().last_modified());
}
+TEST_F(CopyOperationTest, WaitForSyncComplete) {
+ // Create a directory locally.
+ base::FilePath src_path(FILE_PATH_LITERAL("drive/root/File 1.txt"));
+ base::FilePath directory_path(FILE_PATH_LITERAL("drive/root/New Directory"));
+ base::FilePath dest_path = directory_path.AppendASCII("File 1.txt");
+
+ ResourceEntry directory_parent;
+ EXPECT_EQ(FILE_ERROR_OK,
+ GetLocalResourceEntry(directory_path.DirName(), &directory_parent));
+
+ ResourceEntry directory;
+ directory.set_parent_local_id(directory_parent.local_id());
+ directory.set_title(directory_path.BaseName().AsUTF8Unsafe());
+ directory.mutable_file_info()->set_is_directory(true);
+ directory.set_metadata_edit_state(ResourceEntry::DIRTY);
+
+ std::string directory_local_id;
+ FileError error = FILE_ERROR_FAILED;
+ base::PostTaskAndReplyWithResult(
+ blocking_task_runner(),
+ FROM_HERE,
+ base::Bind(&internal::ResourceMetadata::AddEntry,
+ base::Unretained(metadata()), directory, &directory_local_id),
+ google_apis::test_util::CreateCopyResultCallback(&error));
+ content::RunAllBlockingPoolTasksUntilIdle();
+ EXPECT_EQ(FILE_ERROR_OK, error);
+
+ // Try to copy a file to the new directory which lacks resource ID.
+ // This should result in waiting for the directory to sync.
+ std::string waited_local_id;
+ FileOperationCallback pending_callback;
+ observer()->set_wait_for_sync_complete_handler(
+ base::Bind(&CopyWaitForSyncCompleteArguments,
+ &waited_local_id, &pending_callback));
+
+ FileError copy_error = FILE_ERROR_FAILED;
+ operation_->Copy(src_path,
+ dest_path,
+ true, // Preserve last modified.
+ google_apis::test_util::CreateCopyResultCallback(
+ ©_error));
+ content::RunAllBlockingPoolTasksUntilIdle();
+ EXPECT_EQ(directory_local_id, waited_local_id);
+ ASSERT_FALSE(pending_callback.is_null());
+
+ // Add a new directory to the server and store the resource ID locally.
+ google_apis::GDataErrorCode status = google_apis::GDATA_OTHER_ERROR;
+ scoped_ptr<google_apis::FileResource> file_resource;
+ fake_service()->AddNewDirectory(
+ directory_parent.resource_id(),
+ directory.title(),
+ DriveServiceInterface::AddNewDirectoryOptions(),
+ google_apis::test_util::CreateCopyResultCallback(
+ &status, &file_resource));
+ content::RunAllBlockingPoolTasksUntilIdle();
+ EXPECT_EQ(google_apis::HTTP_CREATED, status);
+ ASSERT_TRUE(file_resource);
+
+ directory.set_local_id(directory_local_id);
+ directory.set_resource_id(file_resource->file_id());
+ base::PostTaskAndReplyWithResult(
+ blocking_task_runner(),
+ FROM_HERE,
+ base::Bind(&internal::ResourceMetadata::RefreshEntry,
+ base::Unretained(metadata()), directory),
+ google_apis::test_util::CreateCopyResultCallback(&error));
+ content::RunAllBlockingPoolTasksUntilIdle();
+ EXPECT_EQ(FILE_ERROR_OK, error);
+
+ // Resume the copy operation.
+ pending_callback.Run(FILE_ERROR_OK);
+ content::RunAllBlockingPoolTasksUntilIdle();
+
+ EXPECT_EQ(FILE_ERROR_OK, copy_error);
+ ResourceEntry entry;
+ EXPECT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(dest_path, &entry));
+}
+
} // namespace file_system
} // namespace drive
diff --git a/chrome/browser/chromeos/drive/file_system/operation_observer.cc b/chrome/browser/chromeos/drive/file_system/operation_observer.cc
new file mode 100644
index 0000000..79f0f86
--- /dev/null
+++ b/chrome/browser/chromeos/drive/file_system/operation_observer.cc
@@ -0,0 +1,17 @@
+// Copyright 2014 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/chromeos/drive/file_system/operation_observer.h"
+
+namespace drive {
+namespace file_system {
+
+bool OperationObserver::WaitForSyncComplete(
+ const std::string& local_id,
+ const FileOperationCallback& callback) {
+ return false;
+}
+
+} // namespace file_system
+} // namespace drive
diff --git a/chrome/browser/chromeos/drive/file_system/operation_observer.h b/chrome/browser/chromeos/drive/file_system/operation_observer.h
index 8fd8a1c40..ffd378c 100644
--- a/chrome/browser/chromeos/drive/file_system/operation_observer.h
+++ b/chrome/browser/chromeos/drive/file_system/operation_observer.h
@@ -5,6 +5,8 @@
#ifndef CHROME_BROWSER_CHROMEOS_DRIVE_FILE_SYSTEM_OPERATION_OBSERVER_H_
#define CHROME_BROWSER_CHROMEOS_DRIVE_FILE_SYSTEM_OPERATION_OBSERVER_H_
+#include "chrome/browser/chromeos/drive/file_errors.h"
+
namespace base {
class FilePath;
}
@@ -26,6 +28,7 @@
};
// Passes notifications from Drive operations back to the file system.
+// TODO(hashimoto): Give this class a more appropriate name.
class OperationObserver {
public:
// Sent when a content of a directory has been changed.
@@ -40,6 +43,11 @@
// |local_id| is the local ID of the resource entry.
virtual void OnDriveSyncError(DriveSyncErrorType type,
const std::string& local_id) {}
+
+ // Waits for the sync task to complete and runs the callback.
+ // Returns false if no task is found for the spcecified ID.
+ virtual bool WaitForSyncComplete(const std::string& local_id,
+ const FileOperationCallback& callback);
};
} // namespace file_system
diff --git a/chrome/browser/chromeos/drive/file_system/operation_test_base.cc b/chrome/browser/chromeos/drive/file_system/operation_test_base.cc
index 65af4b6d..9950bca 100644
--- a/chrome/browser/chromeos/drive/file_system/operation_test_base.cc
+++ b/chrome/browser/chromeos/drive/file_system/operation_test_base.cc
@@ -44,6 +44,13 @@
drive_sync_errors_.push_back(type);
}
+bool OperationTestBase::LoggingObserver::WaitForSyncComplete(
+ const std::string& local_id,
+ const FileOperationCallback& callback) {
+ return wait_for_sync_complete_handler_.is_null() ?
+ false : wait_for_sync_complete_handler_.Run(local_id, callback);
+}
+
OperationTestBase::OperationTestBase() {
}
diff --git a/chrome/browser/chromeos/drive/file_system/operation_test_base.h b/chrome/browser/chromeos/drive/file_system/operation_test_base.h
index 26f9d023..7a657e5 100644
--- a/chrome/browser/chromeos/drive/file_system/operation_test_base.h
+++ b/chrome/browser/chromeos/drive/file_system/operation_test_base.h
@@ -48,6 +48,10 @@
// OperationObserver that records all the events.
class LoggingObserver : public OperationObserver {
public:
+ typedef base::Callback<bool(
+ const std::string& local_id,
+ const FileOperationCallback& callback)> WaitForSyncCompleteHandler;
+
LoggingObserver();
~LoggingObserver();
@@ -58,6 +62,9 @@
const std::string& local_id) OVERRIDE;
virtual void OnDriveSyncError(DriveSyncErrorType type,
const std::string& local_id) OVERRIDE;
+ virtual bool WaitForSyncComplete(
+ const std::string& local_id,
+ const FileOperationCallback& callback) OVERRIDE;
// Gets the set of changed paths.
const FileChange& get_changed_files() { return changed_files_; }
@@ -72,10 +79,17 @@
return drive_sync_errors_;
}
+ // Sets the callback used to handle WaitForSyncComplete() method calls.
+ void set_wait_for_sync_complete_handler(
+ const WaitForSyncCompleteHandler& wait_for_sync_complete_handler) {
+ wait_for_sync_complete_handler_ = wait_for_sync_complete_handler;
+ }
+
private:
FileChange changed_files_;
std::set<std::string> updated_local_ids_;
std::vector<DriveSyncErrorType> drive_sync_errors_;
+ WaitForSyncCompleteHandler wait_for_sync_complete_handler_;
};
OperationTestBase();
diff --git a/chrome/browser/chromeos/drive/sync_client.cc b/chrome/browser/chromeos/drive/sync_client.cc
index 52049d52..51b2b42 100644
--- a/chrome/browser/chromeos/drive/sync_client.cc
+++ b/chrome/browser/chromeos/drive/sync_client.cc
@@ -232,6 +232,20 @@
AddUpdateTaskInternal(context, local_id, delay_);
}
+bool SyncClient:: WaitForUpdateTaskToComplete(
+ const std::string& local_id,
+ const FileOperationCallback& callback) {
+ DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+ SyncTasks::iterator it = tasks_.find(SyncTasks::key_type(UPDATE, local_id));
+ if (it == tasks_.end())
+ return false;
+
+ SyncTask* task = &it->second;
+ task->waiting_callbacks.push_back(callback);
+ return true;
+}
+
base::Closure SyncClient::PerformFetchTask(const std::string& local_id,
const ClientContext& context) {
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -439,6 +453,12 @@
<< ": " << FileErrorToString(error);
}
+ for (size_t i = 0; i < it->second.waiting_callbacks.size(); ++i) {
+ base::MessageLoopProxy::current()->PostTask(
+ FROM_HERE, base::Bind(it->second.waiting_callbacks[i], error));
+ }
+ it->second.waiting_callbacks.clear();
+
if (it->second.should_run_again) {
DVLOG(1) << "Running again: type = " << type << ", id = " << local_id;
it->second.state = PENDING;
diff --git a/chrome/browser/chromeos/drive/sync_client.h b/chrome/browser/chromeos/drive/sync_client.h
index 0efccd6..4fc5d1fe 100644
--- a/chrome/browser/chromeos/drive/sync_client.h
+++ b/chrome/browser/chromeos/drive/sync_client.h
@@ -67,6 +67,11 @@
// Adds a update task.
void AddUpdateTask(const ClientContext& context, const std::string& local_id);
+ // Waits for the update task to complete and runs the callback.
+ // Returns false if no task is found for the spcecified ID.
+ bool WaitForUpdateTaskToComplete(const std::string& local_id,
+ const FileOperationCallback& callback);
+
// Starts processing the backlog (i.e. pinned-but-not-filed files and
// dirty-but-not-uploaded files). Kicks off retrieval of the local
// IDs of these files, and then starts the sync loop.
@@ -82,9 +87,6 @@
delay_ = delay;
}
- // Starts the sync loop if it's not running.
- void StartSyncLoop();
-
private:
// Types of sync tasks.
enum SyncType {
@@ -110,6 +112,7 @@
bool should_run_again;
base::Closure cancel_closure;
std::vector<SyncTaskKey> dependent_tasks;
+ std::vector<FileOperationCallback> waiting_callbacks;
};
typedef std::map<SyncTaskKey, SyncTask> SyncTasks;
diff --git a/chrome/browser/chromeos/drive/sync_client_unittest.cc b/chrome/browser/chromeos/drive/sync_client_unittest.cc
index cb56dd28..31000814 100644
--- a/chrome/browser/chromeos/drive/sync_client_unittest.cc
+++ b/chrome/browser/chromeos/drive/sync_client_unittest.cc
@@ -489,5 +489,38 @@
EXPECT_EQ(ResourceEntry::CLEAN, entry2.metadata_edit_state());
}
+TEST_F(SyncClientTest, WaitForUpdateTaskToComplete) {
+ // Create a directory locally.
+ const base::FilePath kPath(FILE_PATH_LITERAL("drive/root/dir1"));
+
+ ResourceEntry parent;
+ EXPECT_EQ(FILE_ERROR_OK,
+ metadata_->GetResourceEntryByPath(kPath.DirName(), &parent));
+
+ ResourceEntry entry;
+ entry.set_parent_local_id(parent.local_id());
+ entry.set_title(kPath.BaseName().AsUTF8Unsafe());
+ entry.mutable_file_info()->set_is_directory(true);
+ entry.set_metadata_edit_state(ResourceEntry::DIRTY);
+ std::string local_id;
+ EXPECT_EQ(FILE_ERROR_OK, metadata_->AddEntry(entry, &local_id));
+
+ // Sync task is not yet avialable.
+ FileError error = FILE_ERROR_FAILED;
+ EXPECT_FALSE(sync_client_->WaitForUpdateTaskToComplete(
+ local_id, google_apis::test_util::CreateCopyResultCallback(&error)));
+
+ // Start syncing the directory and wait for it to complete.
+ sync_client_->AddUpdateTask(ClientContext(USER_INITIATED), local_id);
+
+ EXPECT_TRUE(sync_client_->WaitForUpdateTaskToComplete(
+ local_id, google_apis::test_util::CreateCopyResultCallback(&error)));
+
+ base::RunLoop().RunUntilIdle();
+
+ // The callback is called.
+ EXPECT_EQ(FILE_ERROR_OK, error);
+}
+
} // namespace internal
} // namespace drive
