drive: Respect is_recursive parameter in RemoveOperation.

BUG=223855, 244302

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

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

chrome/browser/chromeos/drive/file_system/remove_operation.cc

@@ -32,22 +32,26 @@ RemoveOperation::~RemoveOperation() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 }
 
-void RemoveOperation::Remove(const base::FilePath& file_path,
+void RemoveOperation::Remove(const base::FilePath& path,
                              bool is_recursive,
                              const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
 
-  // Get the edit URL of an entry at |file_path|.
+  // Get the edit URL of an entry at |path|.
   metadata_->GetResourceEntryByPathOnUIThread(
-      file_path,
+      path,
       base::Bind(
           &RemoveOperation::RemoveAfterGetResourceEntry,
           weak_ptr_factory_.GetWeakPtr(),
+          path,
+          is_recursive,
           callback));
 }
 
 void RemoveOperation::RemoveAfterGetResourceEntry(
+    const base::FilePath& path,
+    bool is_recursive,
     const FileOperationCallback& callback,
     FileError error,
     scoped_ptr<ResourceEntry> entry) {
@@ -58,7 +62,17 @@ void RemoveOperation::RemoveAfterGetResourceEntry(
     callback.Run(error);
     return;
   }
-  DCHECK(entry);
+
+  if (entry->file_info().is_directory() && !is_recursive) {
+    // Check emptiness of the directory.
+    metadata_->ReadDirectoryByPathOnUIThread(
+        path,
+        base::Bind(&RemoveOperation::RemoveAfterReadDirectory,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   entry->resource_id(),
+                   callback));
+    return;
+  }
 
   scheduler_->DeleteResource(
       entry->resource_id(),
@@ -68,6 +82,32 @@ void RemoveOperation::RemoveAfterGetResourceEntry(
                  entry->resource_id()));
 }
 
+void RemoveOperation::RemoveAfterReadDirectory(
+    const std::string& resource_id,
+    const FileOperationCallback& callback,
+    FileError error,
+    scoped_ptr<ResourceEntryVector> entries) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+
+  if (error != FILE_ERROR_OK) {
+    callback.Run(error);
+    return;
+  }
+
+  if (!entries->empty()) {
+    callback.Run(FILE_ERROR_NOT_EMPTY);
+    return;
+  }
+
+  scheduler_->DeleteResource(
+      resource_id,
+      base::Bind(&RemoveOperation::RemoveResourceLocally,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 callback,
+                 resource_id));
+}
+
 void RemoveOperation::RemoveResourceLocally(
     const FileOperationCallback& callback,
     const std::string& resource_id,

chrome/browser/chromeos/drive/file_system/remove_operation.h

@@ -9,6 +9,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/drive/file_errors.h"
+#include "chrome/browser/chromeos/drive/file_system_interface.h"
 #include "chrome/browser/google_apis/gdata_errorcode.h"
 
 namespace base {
@@ -40,27 +41,38 @@ class RemoveOperation {
                   internal::FileCache* cache);
   ~RemoveOperation();
 
-  // Perform the remove operation on the file at drive path |file_path|.
-  // Invokes |callback| when finished with the result of the operation.
+  // Removes the resource at |path|. If |path| is a directory and |is_recursive|
+  // is set, it recursively removes all the descendants. If |is_recursive| is
+  // not set, it succeeds only when the directory is empty.
+  //
   // |callback| must not be null.
-  void Remove(const base::FilePath& file_path,
+  void Remove(const base::FilePath& path,
               bool is_recursive,
               const FileOperationCallback& callback);
 
  private:
   // Part of Remove(). Called after GetResourceEntryByPath() is complete.
-  void RemoveAfterGetResourceEntry(const FileOperationCallback& callback,
+  void RemoveAfterGetResourceEntry(const base::FilePath& path,
+                                   bool is_recursive,
+                                   const FileOperationCallback& callback,
                                    FileError error,
                                    scoped_ptr<ResourceEntry> entry);
 
-  // Callback for DriveServiceInterface::DeleteResource. Removes the entry with
-  // |resource_id| from the local snapshot of the filesystem and the cache.
+  // Part of Remove(). Called when is_recursive = false and trying to remove
+  // a directory. In this case the emptiness of directory must be checked.
+  void RemoveAfterReadDirectory(const std::string& resource_id,
+                                const FileOperationCallback& callback,
+                                FileError error,
+                                scoped_ptr<ResourceEntryVector> entries);
+
+  // Part of Remove(). Called after server-side removal is done. Removes the
+  // entry with |resource_id| from the resource metadata and the cache.
   void RemoveResourceLocally(const FileOperationCallback& callback,
                              const std::string& resource_id,
                              google_apis::GDataErrorCode status);
 
-  // Sends notification for directory changes. Notifies of directory changes,
-  // and runs |callback| with |error|.
+  // Part of Remove(). Sends notification for directory changes, and runs
+  // |callback| with |error|.
   void NotifyDirectoryChanged(const FileOperationCallback& callback,
                               FileError error,
                               const base::FilePath& directory_path);

chrome/browser/chromeos/drive/file_system/remove_operation_unittest.cc

+// Copyright 2013 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/remove_operation.h"
+
+#include "chrome/browser/chromeos/drive/file_system/operation_test_base.h"
+#include "chrome/browser/google_apis/fake_drive_service.h"
+#include "chrome/browser/google_apis/gdata_wapi_parser.h"
+#include "chrome/browser/google_apis/test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace drive {
+namespace file_system {
+
+typedef OperationTestBase RemoveOperationTest;
+
+TEST_F(RemoveOperationTest, RemoveFile) {
+  RemoveOperation operation(observer(), scheduler(), metadata(), cache());
+
+  base::FilePath my_drive(FILE_PATH_LITERAL("drive/root"));
+  base::FilePath nonexisting_file(
+      FILE_PATH_LITERAL("drive/root/Dummy file.txt"));
+  base::FilePath file_in_root(FILE_PATH_LITERAL("drive/root/File 1.txt"));
+  base::FilePath file_in_subdir(
+      FILE_PATH_LITERAL("drive/root/Directory 1/SubDirectory File 1.txt"));
+
+  // Remove a file in root.
+  ResourceEntry entry;
+  FileError error = FILE_ERROR_FAILED;
+  ASSERT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(file_in_root, &entry));
+  operation.Remove(file_in_root,
+                   false,  // is_recursive
+                   google_apis::test_util::CreateCopyResultCallback(&error));
+  google_apis::test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+  EXPECT_EQ(FILE_ERROR_NOT_FOUND, GetLocalResourceEntry(file_in_root, &entry));
+
+  // Remove a file in subdirectory.
+  error = FILE_ERROR_FAILED;
+  ASSERT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(file_in_subdir, &entry));
+  operation.Remove(file_in_subdir,
+                   false,  // is_recursive
+                   google_apis::test_util::CreateCopyResultCallback(&error));
+  google_apis::test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+  EXPECT_EQ(FILE_ERROR_NOT_FOUND,
+            GetLocalResourceEntry(file_in_subdir, &entry));
+
+  // Try removing non-existing file.
+  error = FILE_ERROR_FAILED;
+  ASSERT_EQ(FILE_ERROR_NOT_FOUND,
+            GetLocalResourceEntry(nonexisting_file, &entry));
+  operation.Remove(base::FilePath::FromUTF8Unsafe("drive/root/Dummy file.txt"),
+                   false,  // is_recursive
+                   google_apis::test_util::CreateCopyResultCallback(&error));
+  google_apis::test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_NOT_FOUND, error);
+
+  // Verify observer notifications.
+  EXPECT_EQ(2U, observer()->get_changed_paths().size());
+  EXPECT_TRUE(observer()->get_changed_paths().count(file_in_root.DirName()));
+  EXPECT_TRUE(observer()->get_changed_paths().count(file_in_subdir.DirName()));
+}
+
+TEST_F(RemoveOperationTest, RemoveDirectory) {
+  RemoveOperation operation(observer(), scheduler(), metadata(), cache());
+
+  base::FilePath empty_dir(FILE_PATH_LITERAL(
+      "drive/root/Directory 1/Sub Directory Folder/Sub Sub Directory Folder"));
+  base::FilePath non_empty_dir(FILE_PATH_LITERAL(
+      "drive/root/Directory 1"));
+  base::FilePath file_in_non_empty_dir(FILE_PATH_LITERAL(
+      "drive/root/Directory 1/SubDirectory File 1.txt"));
+
+  // Empty directory can be removed even with is_recursive = false.
+  FileError error = FILE_ERROR_FAILED;
+  ResourceEntry entry;
+  operation.Remove(empty_dir,
+                   false,  // is_recursive
+                   google_apis::test_util::CreateCopyResultCallback(&error));
+  google_apis::test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+  EXPECT_EQ(FILE_ERROR_NOT_FOUND,
+            GetLocalResourceEntry(empty_dir, &entry));
+
+  // Non-empty directory, cannot.
+  error = FILE_ERROR_FAILED;
+  ASSERT_EQ(FILE_ERROR_OK, GetLocalResourceEntry(non_empty_dir, &entry));
+  operation.Remove(non_empty_dir,
+                   false,  // is_recursive
+                   google_apis::test_util::CreateCopyResultCallback(&error));
+  google_apis::test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_NOT_EMPTY, error);
+  EXPECT_EQ(FILE_ERROR_OK,
+            GetLocalResourceEntry(non_empty_dir, &entry));
+
+  // With is_recursive = true, it can be deleted, however. Descendant entries
+  // are removed together.
+  error = FILE_ERROR_FAILED;
+  ASSERT_EQ(FILE_ERROR_OK,
+            GetLocalResourceEntry(file_in_non_empty_dir, &entry));
+  operation.Remove(non_empty_dir,
+                   true,  // is_recursive
+                   google_apis::test_util::CreateCopyResultCallback(&error));
+  google_apis::test_util::RunBlockingPoolTask();
+  EXPECT_EQ(FILE_ERROR_OK, error);
+  EXPECT_EQ(FILE_ERROR_NOT_FOUND,
+            GetLocalResourceEntry(non_empty_dir, &entry));
+  EXPECT_EQ(FILE_ERROR_NOT_FOUND,
+            GetLocalResourceEntry(file_in_non_empty_dir, &entry));
+}
+
+}  // namespace file_system
+}  // namespace drive

chrome/browser/chromeos/drive/file_system_unittest.cc

@@ -172,15 +172,6 @@ class FileSystemTest : public testing::Test {
     return error;
   }
 
-  bool RemoveEntry(const base::FilePath& file_path) {
-    FileError error = FILE_ERROR_FAILED;
-    file_system_->Remove(
-        file_path, false,
-        google_apis::test_util::CreateCopyResultCallback(&error));
-    google_apis::test_util::RunBlockingPoolTask();
-    return error == FILE_ERROR_OK;
-  }
-
   // Gets resource entry by path synchronously.
   scoped_ptr<ResourceEntry> GetResourceEntryByPathSync(
       const base::FilePath& file_path) {
@@ -1136,61 +1127,6 @@ TEST_F(FileSystemTest, CopyFileToInvalidPath) {
   EXPECT_FALSE(EntryExists(dest_file_path));
 }
 
-TEST_F(FileSystemTest, RemoveEntries) {
-  ASSERT_TRUE(LoadRootFeedDocument());
-
-  base::FilePath nonexisting_file(
-      FILE_PATH_LITERAL("drive/root/Dummy file.txt"));
-  base::FilePath file_in_root(FILE_PATH_LITERAL("drive/root/File 1.txt"));
-  base::FilePath dir_in_root(FILE_PATH_LITERAL("drive/root/Directory 1"));
-  base::FilePath file_in_subdir(
-      FILE_PATH_LITERAL("drive/root/Directory 1/SubDirectory File 1.txt"));
-
-  ASSERT_TRUE(EntryExists(file_in_root));
-  scoped_ptr<ResourceEntry> file_in_root_proto = GetResourceEntryByPathSync(
-      file_in_root);
-  ASSERT_TRUE(file_in_root_proto);
-
-  ASSERT_TRUE(EntryExists(dir_in_root));
-  scoped_ptr<ResourceEntry> dir_in_root_proto = GetResourceEntryByPathSync(
-      dir_in_root);
-  ASSERT_TRUE(dir_in_root_proto);
-  ASSERT_TRUE(dir_in_root_proto->file_info().is_directory());
-
-  ASSERT_TRUE(EntryExists(file_in_subdir));
-  scoped_ptr<ResourceEntry> file_in_subdir_proto = GetResourceEntryByPathSync(
-      file_in_subdir);
-  ASSERT_TRUE(file_in_subdir_proto);
-
-  // Once for file in root and once for file...
-  EXPECT_CALL(*mock_directory_observer_, OnDirectoryChanged(
-      Eq(base::FilePath(FILE_PATH_LITERAL("drive/root"))))).Times(2);
-
-  // Remove first file in root.
-  EXPECT_TRUE(RemoveEntry(file_in_root));
-  EXPECT_FALSE(EntryExists(file_in_root));
-  EXPECT_TRUE(EntryExists(dir_in_root));
-  EXPECT_TRUE(EntryExists(file_in_subdir));
-
-  // Remove directory.
-  EXPECT_TRUE(RemoveEntry(dir_in_root));
-  EXPECT_FALSE(EntryExists(file_in_root));
-  EXPECT_FALSE(EntryExists(dir_in_root));
-  EXPECT_FALSE(EntryExists(file_in_subdir));
-
-  // Try removing file in already removed subdirectory.
-  EXPECT_FALSE(RemoveEntry(file_in_subdir));
-
-  // Try removing non-existing file.
-  EXPECT_FALSE(RemoveEntry(nonexisting_file));
-
-  // Try removing root file element.
-  EXPECT_FALSE(RemoveEntry(base::FilePath(FILE_PATH_LITERAL("drive/root"))));
-
-  // Need this to ensure OnDirectoryChanged() is run.
-  google_apis::test_util::RunBlockingPoolTask();
-}
-
 TEST_F(FileSystemTest, CreateDirectory) {
   ASSERT_TRUE(LoadRootFeedDocument());
 

chrome/chrome_tests_unit.gypi

@@ -585,6 +585,7 @@
         'browser/chromeos/drive/file_system/move_operation_unittest.cc',
         'browser/chromeos/drive/file_system/operation_test_base.cc',
         'browser/chromeos/drive/file_system/operation_test_base.h',
+        'browser/chromeos/drive/file_system/remove_operation_unittest.cc',
         'browser/chromeos/drive/file_system/search_operation_unittest.cc',
         'browser/chromeos/drive/file_system/touch_operation_unittest.cc',
         'browser/chromeos/drive/file_system/update_operation_unittest.cc',

chrome/test/data/extensions/api_test/file_browser/filesystem_operations_test/test.js

@@ -436,14 +436,13 @@ function collectTestsForMountPoint(mountPointName, mountPoint) {
   });
 
   testsToRun.push(function deleteDirectoryTest() {
-    // The directory exists if and only if the file system is not drive.
+    // Verify that the directory still exists after the operation.
     var callback = getDirectory.bind(null, mountPoint, 'test_dir', false,
-        !isOnDrive, chrome.test.succeed);
+        true, chrome.test.succeed);
 
     // The directory should still contain some files, so non-recursive delete
-    // should fail. The drive file system does not respect is_recursive flag, so
-    // the operation succeeds.
-    deleteDirectory(mountPoint, 'test_dir', isOnDrive, callback);
+    // should fail.
+    deleteDirectory(mountPoint, 'test_dir', false, callback);
   });
 
   // On drive, the directory was deleted in the previous test.