Implement DeleteFile and DeleteDirectory.

chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc

Index: chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
diff --git a/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc b/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
index b742eea8cf08c89944a53ca2662d18aafde5fba8..006f6398ea6de1e75f2a58da15314b373cf415fd 100644
--- a/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
+++ b/chrome/browser/media_galleries/linux/mtp_device_delegate_impl_linux.cc
@@ -113,6 +113,33 @@ void ReadDirectoryOnUIThread(
   task_helper->ReadDirectory(dir_id, success_callback, error_callback);
 }
 
+// Gets entry ids on |directory_id|.
+//
+// Called on the UI thread to dispatch the request to the
+// MediaTransferProtocolManager.
+//
+// |storage_name| specifies the name of the storage device.
+// |read_only| specifies the mode of the storage device.
+// |directory_id| is the directory to be read.
+// |success_callback| is called when the ReadDirectoryEntryIds request succeeds.
+// |error_callback| is called when the ReadDirectoryEntryIds request fails.
+// |success_callback| and |error_callback| runs on the IO thread.
+void ReadDirectoryEntryIdsOnUIThread(
+    const std::string& storage_name,
+    const bool read_only,
+    const uint32 directory_id,
+    const MTPDeviceTaskHelper::ReadDirectoryEntryIdsSuccessCallback&
+        success_callback,
+    const MTPDeviceTaskHelper::ErrorCallback& error_callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  MTPDeviceTaskHelper* task_helper =
+      GetDeviceTaskHelperForStorage(storage_name, read_only);
+  if (!task_helper)
+    return;
+  task_helper->ReadDirectoryEntryIds(directory_id, success_callback,
+                                     error_callback);
+}
+
 // Gets the |file_path| details.
 //
 // Called on the UI thread to dispatch the request to the
@@ -214,6 +241,31 @@ void CopyFileFromLocalOnUIThread(
                                  error_callback);
 }
 
+// Deletes |object_id|.
+//
+// Called on the UI thread to dispatch the request to the
+// MediaTransferProtocolManager.
+//
+// |storage_name| specifies the name of the storage device.
+// |read_only| specifies the mode of the storage device.
+// |object_id| is the object to be deleted.
+// |success_callback| is called when the object is deleted successfully.
+// |error_callback| is called when it fails to delete the object.
+// |success_callback| and |error_callback| runs on the IO thread.
+void DeleteObjectOnUIThread(
+    const std::string storage_name,
+    const bool read_only,
+    const uint32 object_id,
+    const MTPDeviceTaskHelper::DeleteObjectSuccessCallback success_callback,
+    const MTPDeviceTaskHelper::ErrorCallback error_callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  MTPDeviceTaskHelper* task_helper =
+      GetDeviceTaskHelperForStorage(storage_name, read_only);
+  if (!task_helper)
+    return;
+  task_helper->DeleteObject(object_id, success_callback, error_callback);
+}
+
 // Closes the device storage specified by the |storage_name| and destroys the
 // MTPDeviceTaskHelper object associated with the device storage.
 //
@@ -282,6 +334,8 @@ class MTPDeviceDelegateImplLinux::MTPFileNode {
 
   bool DeleteChild(uint32 file_id);
 
+  bool HasChildren() const;
+
   uint32 file_id() const { return file_id_; }
   const std::string& file_name() const { return file_name_; }
   MTPFileNode* parent() { return parent_; }
@@ -362,6 +416,7 @@ bool MTPDeviceDelegateImplLinux::MTPFileNode::DeleteChild(uint32 file_id) {
   for (ChildNodes::iterator it = children_.begin();
        it != children_.end(); ++it) {
     if (it->second->file_id() == file_id) {
+      DCHECK(!it->second->HasChildren());
       children_.erase(it);
       return true;
     }
@@ -369,6 +424,11 @@ bool MTPDeviceDelegateImplLinux::MTPFileNode::DeleteChild(uint32 file_id) {
   return false;
 }
 
+bool MTPDeviceDelegateImplLinux::MTPFileNode::HasChildren() const {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  return children_.size() > 0;
+}
+
 MTPDeviceDelegateImplLinux::MTPDeviceDelegateImplLinux(
     const std::string& device_location,
     const bool read_only)
@@ -494,7 +554,7 @@ void MTPDeviceDelegateImplLinux::ReadBytes(
                                        closure));
 }
 
-bool MTPDeviceDelegateImplLinux::IsReadOnly() {
+bool MTPDeviceDelegateImplLinux::IsReadOnly() const {
   return read_only_;
 }
 
@@ -520,6 +580,46 @@ void MTPDeviceDelegateImplLinux::CopyFileFromLocal(
                  error_callback));
 }
 
+void MTPDeviceDelegateImplLinux::DeleteFile(
+    const base::FilePath& file_path,
+    const DeleteFileSuccessCallback& success_callback,
+    const ErrorCallback& error_callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DCHECK(!file_path.empty());
+
+  const GetFileInfoSuccessCallback& success_callback_wrapper =
+      base::Bind(&MTPDeviceDelegateImplLinux::DeleteFileInternal,
+                 weak_ptr_factory_.GetWeakPtr(), file_path, success_callback,
+                 error_callback);
+
+  const base::Closure closure =
+      base::Bind(&MTPDeviceDelegateImplLinux::GetFileInfoInternal,
+                 weak_ptr_factory_.GetWeakPtr(), file_path,
+                 success_callback_wrapper, error_callback);
+  EnsureInitAndRunTask(PendingTaskInfo(file_path, content::BrowserThread::IO,
+                                       FROM_HERE, closure));
+}
+
+void MTPDeviceDelegateImplLinux::DeleteDirectory(
+    const base::FilePath& file_path,
+    const DeleteDirectorySuccessCallback& success_callback,
+    const ErrorCallback& error_callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+  DCHECK(!file_path.empty());
+
+  const GetFileInfoSuccessCallback& success_callback_wrapper =
+      base::Bind(&MTPDeviceDelegateImplLinux::DeleteDirectoryInternal,
+                 weak_ptr_factory_.GetWeakPtr(), file_path, success_callback,
+                 error_callback);
+
+  const base::Closure closure =
+      base::Bind(&MTPDeviceDelegateImplLinux::GetFileInfoInternal,
+                 weak_ptr_factory_.GetWeakPtr(), file_path,
+                 success_callback_wrapper, error_callback);
+  EnsureInitAndRunTask(PendingTaskInfo(file_path, content::BrowserThread::IO,
+                                       FROM_HERE, closure));
+}
+
 void MTPDeviceDelegateImplLinux::CancelPendingTasksAndDeleteDelegate() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
   // To cancel all the pending tasks, destroy the MTPDeviceTaskHelper object.
@@ -716,6 +816,111 @@ void MTPDeviceDelegateImplLinux::CopyFileFromLocalInternal(
   PendingRequestDone();
 }
 
+void MTPDeviceDelegateImplLinux::DeleteFileInternal(
+    const base::FilePath& file_path,
+    const DeleteFileSuccessCallback& success_callback,
+    const ErrorCallback& error_callback,
+    const base::File::Info& file_info) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (file_info.is_directory) {
+    error_callback.Run(base::File::FILE_ERROR_NOT_A_FILE);
+  } else {
+    uint32 file_id;
+    if (CachedPathToId(file_path, &file_id))
+      RunDeleteObjectOnUIThread(file_id, success_callback, error_callback);
+    else
+      error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+  }
+
+  PendingRequestDone();
+}
+
+void MTPDeviceDelegateImplLinux::DeleteDirectoryInternal(
+    const base::FilePath& file_path,
+    const DeleteDirectorySuccessCallback& success_callback,
+    const ErrorCallback& error_callback,
+    const base::File::Info& file_info) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (!file_info.is_directory) {
+    error_callback.Run(base::File::FILE_ERROR_NOT_A_DIRECTORY);
+    PendingRequestDone();
+    return;
+  }
+
+  uint32 directory_id;
+  if (!CachedPathToId(file_path, &directory_id)) {
+    error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    PendingRequestDone();
+    return;
+  }
+
+  // Checks the cache first. If it has children in cache, the directory cannot
+  // be empty.
+  FileIdToMTPFileNodeMap::const_iterator it =
+      file_id_to_node_map_.find(directory_id);
+  if (it != file_id_to_node_map_.end() && it->second->HasChildren()) {
+    error_callback.Run(base::File::FILE_ERROR_NOT_EMPTY);
+    PendingRequestDone();
+    return;
+  }
+
+  // Since the directory can contain a file even if the cache returns it as
+  // empty, read the directory and confirm the directory is actually empty.
+  const MTPDeviceTaskHelper::ReadDirectoryEntryIdsSuccessCallback
+      success_callback_wrapper =
+          base::Bind(&MTPDeviceDelegateImplLinux::
+                         OnDidReadDirectoryEntryIdsToDeleteDirectory,
+                     weak_ptr_factory_.GetWeakPtr(), directory_id,
+                     success_callback, error_callback);
+  const MTPDeviceTaskHelper::ErrorCallback error_callback_wrapper =
+      base::Bind(&MTPDeviceDelegateImplLinux::HandleDeviceFileError,
+                 weak_ptr_factory_.GetWeakPtr(), error_callback, directory_id);
+  const base::Closure closure = base::Bind(
+      &ReadDirectoryEntryIdsOnUIThread, storage_name_, read_only_, directory_id,
+      success_callback_wrapper, error_callback_wrapper);
+  EnsureInitAndRunTask(PendingTaskInfo(

[Lei Zhang, 2015/03/10 00:48:06]

This can also just be PostTask().

[yawano, 2015/03/10 05:01:28]

If we use PostTask instead of PendingTaskInfo with empty path
(base::FilePath()), we need to change how we call PendingRequestDone(). The
problem to call PendingRequestDone more often than RunTask is that we allow to
run other operation before a file is actually deleted.

We need to match the number of EnsureInitAndRunTask calls and that of
PendingRequestDone. As far as I see the other codes in this file, it seems to
use PendingTaskInfo with empty path for internal calls.

The current mtpd cannot run operations in parallel, so this should not become a
real problem at now. But to keep the flow simple, I prefer to block the other
operation until a current operation is completed.

Of course, we can use PostTask for internal calls, but it makes
PendingRequestDone far from corresponding EnsureInitAndRunTask. I prefer to keep
them close for readability.

WDYT?

[Lei Zhang, 2015/03/10 08:12:04]

If you were to use PostTask(), then you would not call PendingRequestDone(). The
task that runs on the UI thread would reply, and the callback would either
continue to PostTask() without calling PendingRequestDone(), or finally finish
the overall task at hand (delete in this case) and call PendingRequestDone().

Consistent usage of EnsureInitAndRunTask() and PendingRequestDone() is probably
easier to get correct, so let's go with that.

[yawano, 2015/03/10 08:20:36]

Acknowledged.

+      base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
+  PendingRequestDone();
+}
+
+void MTPDeviceDelegateImplLinux::OnDidReadDirectoryEntryIdsToDeleteDirectory(
+    const uint32 directory_id,
+    const DeleteDirectorySuccessCallback& success_callback,
+    const ErrorCallback& error_callback,
+    const std::vector<uint32>& file_ids) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  if (file_ids.size() > 0)
+    error_callback.Run(base::File::FILE_ERROR_NOT_EMPTY);
+  else
+    RunDeleteObjectOnUIThread(directory_id, success_callback, error_callback);
+
+  PendingRequestDone();
+}
+
+void MTPDeviceDelegateImplLinux::RunDeleteObjectOnUIThread(
+    const uint32 object_id,
+    const DeleteObjectSuccessCallback& success_callback,
+    const ErrorCallback& error_callback) {
+  const MTPDeviceTaskHelper::DeleteObjectSuccessCallback
+      success_callback_wrapper = base::Bind(
+          &MTPDeviceDelegateImplLinux::OnDidDeleteObject,
+          weak_ptr_factory_.GetWeakPtr(), object_id, success_callback);
+
+  const MTPDeviceTaskHelper::ErrorCallback error_callback_wrapper =
+      base::Bind(&MTPDeviceDelegateImplLinux::HandleDeleteFileOrDirectoryError,
+                 weak_ptr_factory_.GetWeakPtr(), error_callback);
+
+  const base::Closure closure =
+      base::Bind(&DeleteObjectOnUIThread, storage_name_, read_only_, object_id,
+                 success_callback_wrapper, error_callback_wrapper);
+
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+                                   closure);
+}
+
 void MTPDeviceDelegateImplLinux::EnsureInitAndRunTask(
     const PendingTaskInfo& task_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
@@ -1022,20 +1227,32 @@ void MTPDeviceDelegateImplLinux::HandleCopyFileFromLocalError(
   PendingRequestDone();
 }
 
+void MTPDeviceDelegateImplLinux::OnDidDeleteObject(
+    const uint32 object_id,
+    const DeleteObjectSuccessCallback success_callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  EvictCachedPathToId(object_id);
+  success_callback.Run();
+  PendingRequestDone();
+}
+
+void MTPDeviceDelegateImplLinux::HandleDeleteFileOrDirectoryError(
+    const ErrorCallback& error_callback,
+    base::File::Error error) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  error_callback.Run(error);
+  PendingRequestDone();
+}
+
 void MTPDeviceDelegateImplLinux::HandleDeviceFileError(
     const ErrorCallback& error_callback,
     uint32 file_id,
     base::File::Error error) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
 
-  FileIdToMTPFileNodeMap::iterator it = file_id_to_node_map_.find(file_id);
-  if (it != file_id_to_node_map_.end()) {
-    MTPFileNode* parent = it->second->parent();
-    if (parent) {
-      bool ret = parent->DeleteChild(file_id);
-      DCHECK(ret);
-    }
-  }
+  EvictCachedPathToId(file_id);
   error_callback.Run(error);
   PendingRequestDone();
 }
@@ -1109,6 +1326,18 @@ bool MTPDeviceDelegateImplLinux::CachedPathToId(const base::FilePath& path,
   return true;
 }
 
+void MTPDeviceDelegateImplLinux::EvictCachedPathToId(const uint32 id) {
+  FileIdToMTPFileNodeMap::iterator it = file_id_to_node_map_.find(id);
+  if (it != file_id_to_node_map_.end()) {
+    DCHECK(!it->second->HasChildren());
+    MTPFileNode* parent = it->second->parent();
+    if (parent) {
+      const bool ret = parent->DeleteChild(id);
+      DCHECK(ret);
+    }
+  }
+}
+
 void CreateMTPDeviceAsyncDelegate(
     const std::string& device_location,
     const bool read_only,