drive: Deal with the root directory same as others in MoveOperation.

Our current implementation of file moving is done in the following logic:
"if(source!=root) RemoveFrom(source); if(target!=root) AddTo(target)"
implicitly assuming "belonging to nowhere" means belonging to root.
But this is not the case.

This patch removes the "if !=root" checks and treats the root as same as other directories.

BUG=169420
TEST=Manually test the steps in the bug report.

Review URL: https://chromiumcodereview.appspot.com/12039005

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

10 files changed, 70 insertions, 57 deletions

diff --git a/chrome/browser/chromeos/drive/drive_file_system_unittest.cc b/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
index e9994d2..f8de023 100644
--- a/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
+++ b/chrome/browser/chromeos/drive/drive_file_system_unittest.cc
@@ -1442,7 +1442,7 @@ TEST_F(DriveFileSystemTest, MoveFileFromRootToSubDirectory) {
 
   // Expect notification for both source and destination directories.
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
-      Eq(FilePath(FILE_PATH_LITERAL("drive"))))).Times(1);
+      Eq(FilePath(FILE_PATH_LITERAL("drive"))))).Times(2);
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
       Eq(FilePath(FILE_PATH_LITERAL("drive/Directory 1"))))).Times(1);
 
@@ -1484,7 +1484,7 @@ TEST_F(DriveFileSystemTest, MoveFileFromSubDirectoryToRoot) {
 
   // Expect notification for both source and destination directories.
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
-      Eq(FilePath(FILE_PATH_LITERAL("drive"))))).Times(1);
+      Eq(FilePath(FILE_PATH_LITERAL("drive"))))).Times(2);
   EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
       Eq(FilePath(FILE_PATH_LITERAL("drive/Directory 1"))))).Times(1);
 
diff --git a/chrome/browser/chromeos/drive/file_system/move_operation.cc b/chrome/browser/chromeos/drive/file_system/move_operation.cc
index 8afb5ee..6392571 100644
--- a/chrome/browser/chromeos/drive/file_system/move_operation.cc
+++ b/chrome/browser/chromeos/drive/file_system/move_operation.cc
@@ -87,19 +87,23 @@ void MoveOperation::MoveAfterGetEntryInfoPair(
   // 1. Renames the file at |src_file_path| to basename(|dest_file_path|)
   //    within the same directory. The rename operation is a no-op if
   //    basename(|src_file_path|) equals to basename(|dest_file_path|).
-  // 2. Removes the file from its parent directory (the file is not deleted),
-  //    which effectively moves the file to the root directory.
-  // 3. Adds the file to the parent directory of |dest_file_path|, which
-  //    effectively moves the file from the root directory to the parent
-  //    directory of |dest_file_path|.
+  // 2. Removes the file from its parent directory (the file is not deleted, but
+  //    just becomes orphaned).
+  // 3. Adds the file to the parent directory of |dest_file_path|.
+  //
+  // TODO(kinaba): After the step 2, the file gets into the state with no parent
+  // node. Our current implementation regards the state as belonging to the root
+  // directory, so below the file is dealt as such. In fact, this is not the
+  // case on the server side. No-parent and in-root is a different concept. We
+  // need to make our implementation consistent to the server: crbug.com/171207.
   const FileMoveCallback add_file_to_directory_callback =
-      base::Bind(&MoveOperation::MoveEntryFromRootDirectory,
+      base::Bind(&MoveOperation::AddEntryToDirectory,
                  weak_ptr_factory_.GetWeakPtr(),
                  dest_parent_path,
                  callback);
 
   const FileMoveCallback remove_file_from_directory_callback =
-      base::Bind(&MoveOperation::RemoveEntryFromNonRootDirectory,
+      base::Bind(&MoveOperation::RemoveEntryFromDirectory,
                  weak_ptr_factory_.GetWeakPtr(),
                  add_file_to_directory_callback);
 
@@ -206,30 +210,23 @@ void MoveOperation::RenameEntryLocally(
                  callback));
 }
 
-void MoveOperation::RemoveEntryFromNonRootDirectory(
+void MoveOperation::RemoveEntryFromDirectory(
     const FileMoveCallback& callback,
     DriveFileError error,
     const FilePath& file_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  const FilePath dir_path = file_path.DirName();
-  // Return if there is an error or |dir_path| is the root directory.
-  if (error != DRIVE_FILE_OK || dir_path == FilePath(kDriveRootDirectory)) {
-    callback.Run(error, file_path);
-    return;
-  }
-
   metadata_->GetEntryInfoPairByPaths(
       file_path,
-      dir_path,
+      file_path.DirName(),
       base::Bind(
-          &MoveOperation::RemoveEntryFromNonRootDirectoryAfterEntryInfoPair,
+          &MoveOperation::RemoveEntryFromDirectoryAfterEntryInfoPair,
           weak_ptr_factory_.GetWeakPtr(),
           callback));
 }
 
-void MoveOperation::RemoveEntryFromNonRootDirectoryAfterEntryInfoPair(
+void MoveOperation::RemoveEntryFromDirectoryAfterEntryInfoPair(
     const FileMoveCallback& callback,
     scoped_ptr<EntryInfoPairResult> result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
@@ -268,34 +265,25 @@ void MoveOperation::RemoveEntryFromNonRootDirectoryAfterEntryInfoPair(
 
 // TODO(zork): Share with CopyOperation.
 // See: crbug.com/150050
-void MoveOperation::MoveEntryFromRootDirectory(
-    const FilePath& directory_path,
-    const FileOperationCallback& callback,
-    DriveFileError error,
-    const FilePath& file_path) {
+void MoveOperation::AddEntryToDirectory(const FilePath& directory_path,
+                                        const FileOperationCallback& callback,
+                                        DriveFileError error,
+                                        const FilePath& file_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
-  DCHECK_EQ(kDriveRootDirectory, file_path.DirName().value());
-
-  // Return if there is an error or |dir_path| is the root directory.
-  if (error != DRIVE_FILE_OK ||
-      directory_path == FilePath(kDriveRootDirectory)) {
-    callback.Run(error);
-    return;
-  }
 
   metadata_->GetEntryInfoPairByPaths(
       file_path,
       directory_path,
       base::Bind(
-          &MoveOperation::MoveEntryFromRootDirectoryAfterGetEntryInfoPair,
+          &MoveOperation::AddEntryToDirectoryAfterGetEntryInfoPair,
           weak_ptr_factory_.GetWeakPtr(),
           callback));
 }
 
 // TODO(zork): Share with CopyOperation.
 // See: crbug.com/150050
-void MoveOperation::MoveEntryFromRootDirectoryAfterGetEntryInfoPair(
+void MoveOperation::AddEntryToDirectoryAfterGetEntryInfoPair(
     const FileOperationCallback& callback,
     scoped_ptr<EntryInfoPairResult> result) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
diff --git a/chrome/browser/chromeos/drive/file_system/move_operation.h b/chrome/browser/chromeos/drive/file_system/move_operation.h
index cf95166..2dd7ca9 100644
--- a/chrome/browser/chromeos/drive/file_system/move_operation.h
+++ b/chrome/browser/chromeos/drive/file_system/move_operation.h
@@ -83,36 +83,34 @@ class MoveOperation {
                           const FileMoveCallback& callback,
                           google_apis::GDataErrorCode status);
 
-  // Removes a file or directory at |file_path| from the current directory if
-  // it's not in the root directory. This essentially moves an entry to the
-  // root directory on the server side.
+  // Removes a file or directory at |file_path| from the current directory.
+  // It moves the entry to a dangle, no-parent state on the server side.
   //
   // Can be called from UI thread. |callback| is run on the calling thread.
   // |callback| must not be null.
-  void RemoveEntryFromNonRootDirectory(const FileMoveCallback& callback,
-                                       DriveFileError error,
-                                       const FilePath& file_path);
+  void RemoveEntryFromDirectory(const FileMoveCallback& callback,
+                                DriveFileError error,
+                                const FilePath& file_path);
 
-  // Part of RemoveEntryFromNonRootDirectory(). Called after
+  // Part of RemoveEntryFromDirectory(). Called after
   // GetEntryInfoPairByPaths() is complete. |callback| must not be null.
-  void RemoveEntryFromNonRootDirectoryAfterEntryInfoPair(
+  void RemoveEntryFromDirectoryAfterEntryInfoPair(
     const FileMoveCallback& callback,
     scoped_ptr<EntryInfoPairResult> result);
 
-  // Moves a file or directory at |file_path| in the root directory to
-  // another directory at |dir_path|. This function does nothing if
-  // |dir_path| points to the root directory.
+  // Moves a file or directory at |file_path| to another directory at
+  // |dir_path|.
   //
   // Can be called from UI thread. |callback| is run on the calling thread.
   // |callback| must not be null.
-  void MoveEntryFromRootDirectory(const FilePath& directory_path,
-                                  const FileOperationCallback& callback,
-                                  DriveFileError error,
-                                  const FilePath& file_path);
+  void AddEntryToDirectory(const FilePath& directory_path,
+                           const FileOperationCallback& callback,
+                           DriveFileError error,
+                           const FilePath& file_path);
 
-  // Part of MoveEntryFromRootDirectory(). Called after
+  // Part of AddEntryToDirectory(). Called after
   // GetEntryInfoPairByPaths() is complete. |callback| must not be null.
-  void MoveEntryFromRootDirectoryAfterGetEntryInfoPair(
+  void AddEntryToDirectoryAfterGetEntryInfoPair(
     const FileOperationCallback& callback,
     scoped_ptr<EntryInfoPairResult> result);
 
diff --git a/chrome/browser/google_apis/fake_drive_service.cc b/chrome/browser/google_apis/fake_drive_service.cc
index 80990c9..7eb3030 100644
--- a/chrome/browser/google_apis/fake_drive_service.cc
+++ b/chrome/browser/google_apis/fake_drive_service.cc
@@ -581,6 +581,14 @@ void FakeDriveService::RemoveResourceFromDirectory(
           return;
         }
       }
+
+      // We are dealing with a no-"parent"-link file as in the root directory.
+      if (parent_content_url.is_empty()) {
+        AddNewChangestamp(entry);
+        MessageLoop::current()->PostTask(
+            FROM_HERE, base::Bind(callback, HTTP_SUCCESS));
+        return;
+      }
     }
   }
 
diff --git a/chrome/browser/google_apis/gdata_wapi_operations.cc b/chrome/browser/google_apis/gdata_wapi_operations.cc
index 09e7abb..2c865f7 100644
--- a/chrome/browser/google_apis/gdata_wapi_operations.cc
+++ b/chrome/browser/google_apis/gdata_wapi_operations.cc
@@ -489,10 +489,9 @@ AddResourceToDirectoryOperation::AddResourceToDirectoryOperation(
 AddResourceToDirectoryOperation::~AddResourceToDirectoryOperation() {}
 
 GURL AddResourceToDirectoryOperation::GetURL() const {
-  if (!parent_content_url_.is_empty())
-    return GDataWapiUrlGenerator::AddStandardUrlParams(parent_content_url_);
-
-  return url_generator_.GenerateResourceListRootUrl();
+  GURL parent = parent_content_url_.is_empty() ?
+      url_generator_.GenerateRootContentUrl() : parent_content_url_;
+  return GDataWapiUrlGenerator::AddStandardUrlParams(parent);
 }
 
 URLFetcher::RequestType
@@ -523,10 +522,12 @@ bool AddResourceToDirectoryOperation::GetContentData(
 RemoveResourceFromDirectoryOperation::RemoveResourceFromDirectoryOperation(
     OperationRegistry* registry,
     net::URLRequestContextGetter* url_request_context_getter,
+    const GDataWapiUrlGenerator& url_generator,
     const EntryActionCallback& callback,
     const GURL& parent_content_url,
     const std::string& document_resource_id)
     : EntryActionOperation(registry, url_request_context_getter, callback),
+      url_generator_(url_generator),
       resource_id_(document_resource_id),
       parent_content_url_(parent_content_url) {
   DCHECK(!callback.is_null());
@@ -536,9 +537,12 @@ RemoveResourceFromDirectoryOperation::~RemoveResourceFromDirectoryOperation() {
 }
 
 GURL RemoveResourceFromDirectoryOperation::GetURL() const {
+  GURL parent = parent_content_url_.is_empty() ?
+      url_generator_.GenerateRootContentUrl() : parent_content_url_;
+
   std::string escaped_resource_id = net::EscapePath(resource_id_);
   GURL edit_url(base::StringPrintf("%s/%s",
-                                   parent_content_url_.spec().c_str(),
+                                   parent.spec().c_str(),
                                    escaped_resource_id.c_str()));
   return GDataWapiUrlGenerator::AddStandardUrlParams(edit_url);
 }
diff --git a/chrome/browser/google_apis/gdata_wapi_operations.h b/chrome/browser/google_apis/gdata_wapi_operations.h
index e122f8f..13e2c2d 100644
--- a/chrome/browser/google_apis/gdata_wapi_operations.h
+++ b/chrome/browser/google_apis/gdata_wapi_operations.h
@@ -378,6 +378,7 @@ class RemoveResourceFromDirectoryOperation : public EntryActionOperation {
   RemoveResourceFromDirectoryOperation(
       OperationRegistry* registry,
       net::URLRequestContextGetter* url_request_context_getter,
+      const GDataWapiUrlGenerator& url_generator,
       const EntryActionCallback& callback,
       const GURL& parent_content_url,
       const std::string& resource_id);
@@ -390,6 +391,7 @@ class RemoveResourceFromDirectoryOperation : public EntryActionOperation {
   virtual std::vector<std::string> GetExtraRequestHeaders() const OVERRIDE;
 
  private:
+  const GDataWapiUrlGenerator url_generator_;
   const std::string resource_id_;
   const GURL parent_content_url_;
 
diff --git a/chrome/browser/google_apis/gdata_wapi_operations_unittest.cc b/chrome/browser/google_apis/gdata_wapi_operations_unittest.cc
index 92d7272..c0e5b22 100644
--- a/chrome/browser/google_apis/gdata_wapi_operations_unittest.cc
+++ b/chrome/browser/google_apis/gdata_wapi_operations_unittest.cc
@@ -824,6 +824,7 @@ TEST_F(GDataWapiOperationsTest, RemoveResourceFromDirectoryOperation) {
       new RemoveResourceFromDirectoryOperation(
           &operation_registry_,
           request_context_getter_.get(),
+          *url_generator_,
           base::Bind(&CopyResultFromEntryActionCallbackAndQuit,
                      &result_code),
           test_server_.GetURL("/feeds/default/private/full/folder%3Aroot"),
diff --git a/chrome/browser/google_apis/gdata_wapi_service.cc b/chrome/browser/google_apis/gdata_wapi_service.cc
index ce42a39..3d4346e 100644
--- a/chrome/browser/google_apis/gdata_wapi_service.cc
+++ b/chrome/browser/google_apis/gdata_wapi_service.cc
@@ -452,6 +452,7 @@ void GDataWapiService::RemoveResourceFromDirectory(
   runner_->StartOperationWithRetry(
       new RemoveResourceFromDirectoryOperation(operation_registry(),
                                                url_request_context_getter_,
+                                               url_generator_,
                                                callback,
                                                parent_content_url,
                                                resource_id));
diff --git a/chrome/browser/google_apis/gdata_wapi_url_generator.cc b/chrome/browser/google_apis/gdata_wapi_url_generator.cc
index ac38ef6..527ec49 100644
--- a/chrome/browser/google_apis/gdata_wapi_url_generator.cc
+++ b/chrome/browser/google_apis/gdata_wapi_url_generator.cc
@@ -32,6 +32,10 @@ const char kResourceListRootURL[] = "/feeds/default/private/full";
 // Metadata feed with things like user quota.
 const char kAccountMetadataURL[] = "/feeds/metadata/default";
 
+// URL for the content_url of the root directory.
+const char kRootContentURL[] =
+    "/feeds/default/private/full/folder%3Aroot/contents";
+
 #ifndef NDEBUG
 // Use smaller 'page' size while debugging to ensure we hit feed reload
 // almost always. Be careful not to use something too small on account that
@@ -161,4 +165,8 @@ GURL GDataWapiUrlGenerator::GenerateAccountMetadataUrl() const {
   return AddMetadataUrlParams(base_url_.Resolve(kAccountMetadataURL));
 }
 
+GURL GDataWapiUrlGenerator::GenerateRootContentUrl() const {
+  return base_url_.Resolve(kRootContentURL);
+}
+
 }  // namespace google_apis
diff --git a/chrome/browser/google_apis/gdata_wapi_url_generator.h b/chrome/browser/google_apis/gdata_wapi_url_generator.h
index 5700ba7c..0717188 100644
--- a/chrome/browser/google_apis/gdata_wapi_url_generator.h
+++ b/chrome/browser/google_apis/gdata_wapi_url_generator.h
@@ -89,6 +89,9 @@ class GDataWapiUrlGenerator {
   // Generates a URL for getting the account metadata feed.
   GURL GenerateAccountMetadataUrl() const;
 
+  // Generates a URL for getting the root directory's content URL.
+  GURL GenerateRootContentUrl() const;
+
  private:
   const GURL base_url_;
 };