Reimplement MoveOperation.

Reimplement MoveOperation.

The main goal of this CL is fixing the condition to skip Rename().
Instead of adding more complicated condition to MoveOperation, this CL
simplifies the whole MoveOperation implementation.
This is also the preparation to use DriveServiceInterface::MoveResource on
Drive API v2.

BUG=241814
TEST=Ran unit_tests and tested manually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=218789

[hidehiko, 2013-08-21 07:22:21]

Thank you for your review in advance,
- hidehiko

[kinaba, 2013-08-21 07:37:22]

lgtm!

I guess you can remove many methods from ResourceMetadata (as a separate patch).

[hashimoto, 2013-08-21 07:49:08]

lgtm

https://codereview.chromium.org/22920004/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/move_operation.cc (right):

https://codereview.chromium.org/22920004/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/move_operation.cc:25:
DCHECK(metadata);
nit: Do we need this? At least, two functions below are not doing this.

https://codereview.chromium.org/22920004/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/move_operation.cc:58: // Make sure
that the parent exists and it is actually a directory.
ResourceMetadata::RefreshEntry does this check.

[hidehiko, 2013-08-21 08:07:00]

Thank you for your review. PTAL?

https://codereview.chromium.org/22920004/diff/1/chrome/browser/chromeos/drive...
File chrome/browser/chromeos/drive/file_system/move_operation.cc (right):

https://codereview.chromium.org/22920004/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/move_operation.cc:25:
DCHECK(metadata);
On 2013/08/21 07:49:08, hashimoto wrote:
> nit: Do we need this? At least, two functions below are not doing this.

Ok, removed.

https://codereview.chromium.org/22920004/diff/1/chrome/browser/chromeos/drive...
chrome/browser/chromeos/drive/file_system/move_operation.cc:58: // Make sure
that the parent exists and it is actually a directory.
On 2013/08/21 07:49:08, hashimoto wrote:
> ResourceMetadata::RefreshEntry does this check.

Removed. Instead, edit ResourceMetadata::RefreshEntry a bit in order to return
FILE_ERROR_NOT_A_DIRECTORY.

[hashimoto, 2013-08-21 08:12:44]

lgtm, thanks

[commit-bot: I haz the power, 2013-08-21 08:14:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/22920004/7001

[commit-bot: I haz the power, 2013-08-21 08:14:43]

Failed to apply patch for
chrome/browser/chromeos/drive/file_system/move_operation.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/file_system/move_operation.cc
  Hunk #2 FAILED at 79.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/file_system/move_operation.cc.rej

Patch:       chrome/browser/chromeos/drive/file_system/move_operation.cc
Index: chrome/browser/chromeos/drive/file_system/move_operation.cc
diff --git a/chrome/browser/chromeos/drive/file_system/move_operation.cc
b/chrome/browser/chromeos/drive/file_system/move_operation.cc
index
ded4aacddd9668d48da29b44169c80364f35dfb3..6c6c6aa1791b43c37a45a9a57b068949f1e4d115
100644
--- a/chrome/browser/chromeos/drive/file_system/move_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/move_operation.cc
@@ -14,14 +14,58 @@ using content::BrowserThread;
 
 namespace drive {
 namespace file_system {
+namespace {
+
+// Looks up ResourceEntry for source entry and the destination directory.
+FileError PrepareMove(internal::ResourceMetadata* metadata,
+                      const base::FilePath& src_path,
+                      const base::FilePath& dest_parent_path,
+                      ResourceEntry* src_entry,
+                      ResourceEntry* dest_parent_entry) {
+  FileError error = metadata->GetResourceEntryByPath(src_path, src_entry);
+  if (error != FILE_ERROR_OK)
+    return error;
+
+  return metadata->GetResourceEntryByPath(dest_parent_path, dest_parent_entry);
+}
+
+// Applies renaming to the local metadata.
+FileError RenameLocally(internal::ResourceMetadata* metadata,
+                        const std::string& resource_id,
+                        const std::string& new_title) {
+  ResourceEntry entry;
+  FileError error = metadata->GetResourceEntryById(resource_id, &entry);
+  if (error != FILE_ERROR_OK)
+    return error;
+
+  entry.set_title(new_title);
+  return metadata->RefreshEntry(entry);
+}
+
+// Applies directory-moving to the local metadata.
+FileError MoveDirectoryLocally(internal::ResourceMetadata* metadata,
+                               const std::string& resource_id,
+                               const std::string& parent_resource_id) {
+  ResourceEntry entry;
+  FileError error = metadata->GetResourceEntryById(resource_id, &entry);
+  if (error != FILE_ERROR_OK)
+    return error;
+
+  entry.set_parent_resource_id(parent_resource_id);
+  return metadata->RefreshEntry(entry);
+}
 
-MoveOperation::MoveOperation(OperationObserver* observer,
+}  // namespace
+
+MoveOperation::MoveOperation(base::SequencedTaskRunner* blocking_task_runner,
+                             OperationObserver* observer,
                              JobScheduler* scheduler,
                              internal::ResourceMetadata* metadata)
-  : observer_(observer),
-    scheduler_(scheduler),
-    metadata_(metadata),
-    weak_ptr_factory_(this) {
+    : blocking_task_runner_(blocking_task_runner),
+      observer_(observer),
+      scheduler_(scheduler),
+      metadata_(metadata),
+      weak_ptr_factory_(this) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -35,175 +79,198 @@ void MoveOperation::Move(const base::FilePath&
src_file_path,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  metadata_->GetResourceEntryPairByPathsOnUIThread(
-      src_file_path,
-      dest_file_path.DirName(),
-      base::Bind(&MoveOperation::MoveAfterGetResourceEntryPair,
+  scoped_ptr<ResourceEntry> src_entry(new ResourceEntry);
+  scoped_ptr<ResourceEntry> dest_parent_entry(new ResourceEntry);
+  ResourceEntry* src_entry_ptr = src_entry.get();
+  ResourceEntry* dest_parent_entry_ptr = dest_parent_entry.get();
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&PrepareMove,
+                 metadata_, src_file_path, dest_file_path.DirName(),
+                 src_entry_ptr, dest_parent_entry_ptr),
+      base::Bind(&MoveOperation::MoveAfterPrepare,
                  weak_ptr_factory_.GetWeakPtr(),
-                 dest_file_path,
-                 callback));
+                 src_file_path, dest_file_path, callback,
+                 base::Passed(&src_entry),
+                 base::Passed(&dest_parent_entry)));
 }
 
-void MoveOperation::MoveAfterGetResourceEntryPair(
+void MoveOperation::MoveAfterPrepare(
+    const base::FilePath& src_file_path,
     const base::FilePath& dest_file_path,
     const FileOperationCallback& callback,
-    scoped_ptr<EntryInfoPairResult> src_dest_info) {
+    scoped_ptr<ResourceEntry> src_entry,
+    scoped_ptr<ResourceEntry> dest_parent_entry,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
-  DCHECK(src_dest_info.get());
 
-  if (src_dest_info->first.error != FILE_ERROR_OK) {
-    callback.Run(src_dest_info->first.error);
-    return;
-  }
-  if (src_dest_info->second.error != FILE_ERROR_OK) {
-    callback.Run(src_dest_info->second.error);
+  if (error != FILE_ERROR_OK) {
+    callback.Run(error);
     return;
   }
-  if (!src_dest_info->second.entry->file_info().is_directory()) {
+
+  if (!dest_parent_entry->file_info().is_directory()) {
+    // The parent of the destination is not a directory.
     callback.Run(FILE_ERROR_NOT_A_DIRECTORY);
     return;
   }
 
-  const std::string& src_id = src_dest_info->first.entry->resource_id();
-  const base::FilePath& src_path = src_dest_info->first.path;
-  const base::FilePath new_name = dest_file_path.BaseName();
-  const bool new_name_has_hosted_extension =
-      src_dest_info->first.entry->has_file_specific_info() &&
-      src_dest_info->first.entry->file_specific_info().is_hosted_document() &&
-      new_name.Extension() ==
-         
src_dest_info->first.entry->file_specific_info().document_extension();
-
-  Rename(src_id, src_path, new_name, new_name_has_hosted_extension,
+  // Strip the extension for a hosted document if necessary.
+  const bool has_hosted_document_extension =
+      src_entry->has_file_specific_info() &&
+      src_entry->file_specific_info().is_hosted_document() &&
+      dest_file_path.Extension() ==
+      src_entry->file_specific_info().document_extension();
+  const std::string new_title =
+      has_hosted_document_extension ?
+      dest_file_path.BaseName().RemoveExtension().AsUTF8Unsafe() :
+      dest_file_path.BaseName().AsUTF8Unsafe();
+
+  // TODO(hidehiko): On Drive API v2, we can move a resource by only one
+  // server request. Implement here. crbug.com/241814
+
+  ResourceEntry* src_entry_ptr = src_entry.get();
+  Rename(*src_entry_ptr, new_title,
          base::Bind(&MoveOperation::MoveAfterRename,
                     weak_ptr_factory_.GetWeakPtr(),
-                    callback,
-                    base::Passed(&src_dest_info)));
+                    src_file_path, dest_file_path, callback,
+                    base::Passed(&src_entry),
+                    base::Passed(&dest_parent_entry)));
 }
 
 void MoveOperation::MoveAfterRename(
+    const base::FilePath& src_file_path,
+    const base::FilePath& dest_file_path,
     const FileOperationCallback& callback,
-    scoped_ptr<EntryInfoPairResult> src_dest_info,
-    FileError error,
-    const base::FilePath& src_path) {
+    scoped_ptr<ResourceEntry> src_entry,
+    scoped_ptr<ResourceEntry> dest_parent_entry,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
   if (error != FILE_ERROR_OK) {
     callback.Run(error);
     return;
   }
 
-  const std::string& src_id = src_dest_info->first.entry->resource_id();
-  const std::string& dest_dir_id = src_dest_info->second.entry->resource_id();
-  const base::FilePath& dest_dir_path = src_dest_info->second.path;
-
   // The source and the destination directory are the same. Nothing more to do.
-  if (src_path.DirName() == dest_dir_path) {
-    observer_->OnDirectoryChangedByOperation(dest_dir_path);
+  if (src_entry->parent_resource_id() == dest_parent_entry->resource_id()) {
+    observer_->OnDirectoryChangedByOperation(dest_file_path.DirName());
     callback.Run(FILE_ERROR_OK);
     return;
   }
 
-  AddToDirectory(src_id, dest_dir_id, src_path, dest_dir_path,
+  AddToDirectory(src_entry->resource_id(),
+                 dest_parent_entry->resource_id(),
                  base::Bind(&MoveOperation::MoveAfterAddToDirectory,
                             weak_ptr_factory_.GetWeakPtr(),
-                            callback,
-                            base::Passed(&src_dest_info)));
+                            src_file_path, dest_file_path, callback,
+                            src_entry->resource_id(),
+                            src_entry->parent_resource_id()));
 }
 
 void MoveOperation::MoveAfterAddToDirectory(
+    const base::FilePath& src_file_path,
+    const base::FilePath& dest_file_path,
     const FileOperationCallback& callback,
-    scoped_ptr<EntryInfoPairResult> src_dest_info,
-    FileError error,
-    const base::FilePath& new_path) {
+    const std::string& resource_id,
+    const std::string& parent_resource_id,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
   if (error != FILE_ERROR_OK) {
     callback.Run(error);
     return;
   }
 
-  const base::FilePath& src_path = src_dest_info->first.path;
-  observer_->OnDirectoryChangedByOperation(src_path.DirName());
-  observer_->OnDirectoryChangedByOperation(new_path.DirName());
+  // Notify to the observers.
+  observer_->OnDirectoryChangedByOperation(src_file_path.DirName());
+  observer_->OnDirectoryChangedByOperation(dest_file_path.DirName());
 
-  RemoveFromDirectory(src_dest_info->first.entry->resource_id(),
-                      src_dest_info->first.entry->parent_resource_id(),
-                      callback);
+  Remo…
(message too large)

[commit-bot: I haz the power, 2013-08-21 10:19:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/22920004/7001

[commit-bot: I haz the power, 2013-08-21 10:19:37]

Failed to apply patch for
chrome/browser/chromeos/drive/file_system/move_operation.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/file_system/move_operation.cc
  Hunk #2 FAILED at 79.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/file_system/move_operation.cc.rej

Patch:       chrome/browser/chromeos/drive/file_system/move_operation.cc
Index: chrome/browser/chromeos/drive/file_system/move_operation.cc
diff --git a/chrome/browser/chromeos/drive/file_system/move_operation.cc
b/chrome/browser/chromeos/drive/file_system/move_operation.cc
index
ded4aacddd9668d48da29b44169c80364f35dfb3..6c6c6aa1791b43c37a45a9a57b068949f1e4d115
100644
--- a/chrome/browser/chromeos/drive/file_system/move_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/move_operation.cc
@@ -14,14 +14,58 @@ using content::BrowserThread;
 
 namespace drive {
 namespace file_system {
+namespace {
+
+// Looks up ResourceEntry for source entry and the destination directory.
+FileError PrepareMove(internal::ResourceMetadata* metadata,
+                      const base::FilePath& src_path,
+                      const base::FilePath& dest_parent_path,
+                      ResourceEntry* src_entry,
+                      ResourceEntry* dest_parent_entry) {
+  FileError error = metadata->GetResourceEntryByPath(src_path, src_entry);
+  if (error != FILE_ERROR_OK)
+    return error;
+
+  return metadata->GetResourceEntryByPath(dest_parent_path, dest_parent_entry);
+}
+
+// Applies renaming to the local metadata.
+FileError RenameLocally(internal::ResourceMetadata* metadata,
+                        const std::string& resource_id,
+                        const std::string& new_title) {
+  ResourceEntry entry;
+  FileError error = metadata->GetResourceEntryById(resource_id, &entry);
+  if (error != FILE_ERROR_OK)
+    return error;
+
+  entry.set_title(new_title);
+  return metadata->RefreshEntry(entry);
+}
+
+// Applies directory-moving to the local metadata.
+FileError MoveDirectoryLocally(internal::ResourceMetadata* metadata,
+                               const std::string& resource_id,
+                               const std::string& parent_resource_id) {
+  ResourceEntry entry;
+  FileError error = metadata->GetResourceEntryById(resource_id, &entry);
+  if (error != FILE_ERROR_OK)
+    return error;
+
+  entry.set_parent_resource_id(parent_resource_id);
+  return metadata->RefreshEntry(entry);
+}
 
-MoveOperation::MoveOperation(OperationObserver* observer,
+}  // namespace
+
+MoveOperation::MoveOperation(base::SequencedTaskRunner* blocking_task_runner,
+                             OperationObserver* observer,
                              JobScheduler* scheduler,
                              internal::ResourceMetadata* metadata)
-  : observer_(observer),
-    scheduler_(scheduler),
-    metadata_(metadata),
-    weak_ptr_factory_(this) {
+    : blocking_task_runner_(blocking_task_runner),
+      observer_(observer),
+      scheduler_(scheduler),
+      metadata_(metadata),
+      weak_ptr_factory_(this) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
@@ -35,175 +79,198 @@ void MoveOperation::Move(const base::FilePath&
src_file_path,
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  metadata_->GetResourceEntryPairByPathsOnUIThread(
-      src_file_path,
-      dest_file_path.DirName(),
-      base::Bind(&MoveOperation::MoveAfterGetResourceEntryPair,
+  scoped_ptr<ResourceEntry> src_entry(new ResourceEntry);
+  scoped_ptr<ResourceEntry> dest_parent_entry(new ResourceEntry);
+  ResourceEntry* src_entry_ptr = src_entry.get();
+  ResourceEntry* dest_parent_entry_ptr = dest_parent_entry.get();
+  base::PostTaskAndReplyWithResult(
+      blocking_task_runner_.get(),
+      FROM_HERE,
+      base::Bind(&PrepareMove,
+                 metadata_, src_file_path, dest_file_path.DirName(),
+                 src_entry_ptr, dest_parent_entry_ptr),
+      base::Bind(&MoveOperation::MoveAfterPrepare,
                  weak_ptr_factory_.GetWeakPtr(),
-                 dest_file_path,
-                 callback));
+                 src_file_path, dest_file_path, callback,
+                 base::Passed(&src_entry),
+                 base::Passed(&dest_parent_entry)));
 }
 
-void MoveOperation::MoveAfterGetResourceEntryPair(
+void MoveOperation::MoveAfterPrepare(
+    const base::FilePath& src_file_path,
     const base::FilePath& dest_file_path,
     const FileOperationCallback& callback,
-    scoped_ptr<EntryInfoPairResult> src_dest_info) {
+    scoped_ptr<ResourceEntry> src_entry,
+    scoped_ptr<ResourceEntry> dest_parent_entry,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
-  DCHECK(src_dest_info.get());
 
-  if (src_dest_info->first.error != FILE_ERROR_OK) {
-    callback.Run(src_dest_info->first.error);
-    return;
-  }
-  if (src_dest_info->second.error != FILE_ERROR_OK) {
-    callback.Run(src_dest_info->second.error);
+  if (error != FILE_ERROR_OK) {
+    callback.Run(error);
     return;
   }
-  if (!src_dest_info->second.entry->file_info().is_directory()) {
+
+  if (!dest_parent_entry->file_info().is_directory()) {
+    // The parent of the destination is not a directory.
     callback.Run(FILE_ERROR_NOT_A_DIRECTORY);
     return;
   }
 
-  const std::string& src_id = src_dest_info->first.entry->resource_id();
-  const base::FilePath& src_path = src_dest_info->first.path;
-  const base::FilePath new_name = dest_file_path.BaseName();
-  const bool new_name_has_hosted_extension =
-      src_dest_info->first.entry->has_file_specific_info() &&
-      src_dest_info->first.entry->file_specific_info().is_hosted_document() &&
-      new_name.Extension() ==
-         
src_dest_info->first.entry->file_specific_info().document_extension();
-
-  Rename(src_id, src_path, new_name, new_name_has_hosted_extension,
+  // Strip the extension for a hosted document if necessary.
+  const bool has_hosted_document_extension =
+      src_entry->has_file_specific_info() &&
+      src_entry->file_specific_info().is_hosted_document() &&
+      dest_file_path.Extension() ==
+      src_entry->file_specific_info().document_extension();
+  const std::string new_title =
+      has_hosted_document_extension ?
+      dest_file_path.BaseName().RemoveExtension().AsUTF8Unsafe() :
+      dest_file_path.BaseName().AsUTF8Unsafe();
+
+  // TODO(hidehiko): On Drive API v2, we can move a resource by only one
+  // server request. Implement here. crbug.com/241814
+
+  ResourceEntry* src_entry_ptr = src_entry.get();
+  Rename(*src_entry_ptr, new_title,
          base::Bind(&MoveOperation::MoveAfterRename,
                     weak_ptr_factory_.GetWeakPtr(),
-                    callback,
-                    base::Passed(&src_dest_info)));
+                    src_file_path, dest_file_path, callback,
+                    base::Passed(&src_entry),
+                    base::Passed(&dest_parent_entry)));
 }
 
 void MoveOperation::MoveAfterRename(
+    const base::FilePath& src_file_path,
+    const base::FilePath& dest_file_path,
     const FileOperationCallback& callback,
-    scoped_ptr<EntryInfoPairResult> src_dest_info,
-    FileError error,
-    const base::FilePath& src_path) {
+    scoped_ptr<ResourceEntry> src_entry,
+    scoped_ptr<ResourceEntry> dest_parent_entry,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
   if (error != FILE_ERROR_OK) {
     callback.Run(error);
     return;
   }
 
-  const std::string& src_id = src_dest_info->first.entry->resource_id();
-  const std::string& dest_dir_id = src_dest_info->second.entry->resource_id();
-  const base::FilePath& dest_dir_path = src_dest_info->second.path;
-
   // The source and the destination directory are the same. Nothing more to do.
-  if (src_path.DirName() == dest_dir_path) {
-    observer_->OnDirectoryChangedByOperation(dest_dir_path);
+  if (src_entry->parent_resource_id() == dest_parent_entry->resource_id()) {
+    observer_->OnDirectoryChangedByOperation(dest_file_path.DirName());
     callback.Run(FILE_ERROR_OK);
     return;
   }
 
-  AddToDirectory(src_id, dest_dir_id, src_path, dest_dir_path,
+  AddToDirectory(src_entry->resource_id(),
+                 dest_parent_entry->resource_id(),
                  base::Bind(&MoveOperation::MoveAfterAddToDirectory,
                             weak_ptr_factory_.GetWeakPtr(),
-                            callback,
-                            base::Passed(&src_dest_info)));
+                            src_file_path, dest_file_path, callback,
+                            src_entry->resource_id(),
+                            src_entry->parent_resource_id()));
 }
 
 void MoveOperation::MoveAfterAddToDirectory(
+    const base::FilePath& src_file_path,
+    const base::FilePath& dest_file_path,
     const FileOperationCallback& callback,
-    scoped_ptr<EntryInfoPairResult> src_dest_info,
-    FileError error,
-    const base::FilePath& new_path) {
+    const std::string& resource_id,
+    const std::string& parent_resource_id,
+    FileError error) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
 
   if (error != FILE_ERROR_OK) {
     callback.Run(error);
     return;
   }
 
-  const base::FilePath& src_path = src_dest_info->first.path;
-  observer_->OnDirectoryChangedByOperation(src_path.DirName());
-  observer_->OnDirectoryChangedByOperation(new_path.DirName());
+  // Notify to the observers.
+  observer_->OnDirectoryChangedByOperation(src_file_path.DirName());
+  observer_->OnDirectoryChangedByOperation(dest_file_path.DirName());
 
-  RemoveFromDirectory(src_dest_info->first.entry->resource_id(),
-                      src_dest_info->first.entry->parent_resource_id(),
-                      callback);
+  Remo…
(message too large)

[hidehiko, 2013-08-21 16:20:30]

Rebased and resolved conflict. Sending to CQ again...

[commit-bot: I haz the power, 2013-08-21 16:20:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/22920004/22001

[commit-bot: I haz the power, 2013-08-21 19:02:22]

Change committed as 218789