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..ab76739efb85f0c51a42cafafd88b437c456c3b4 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(

[Lei Zhang, 2015/03/07 01:17:24]

What if we:
- got rid of this
- make ReadDirectoryOnUIThread() take an optional chunk size. (0 = default)
-- plumb that parameter down to MediaTransferProtocolManagerImpl
-- get rid the newly added ReadDirectoryEntryIds code
- Call ReadDirectoryOnUIThread(chunk_size = 1) instead of
ReadDirectoryEntryIdsOnUIThread()

Would that save us some code?

[yawano, 2015/03/09 03:46:48]

Can I confirm that chunk_size is maximum_length? I think there is no way to
cancel read directory call now.

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

Actually, my idea only works if we modify
MediaTransferProtocolManagerImpl::OnGotDirectoryEntries() to read at most 1
entry, not a chunk of 1 entry - as it, it would keep reading more directory
entries. Since we can't cancel, we need some way of telling the read directory
call to end early. WDYT?

[yawano, 2015/03/10 01:17:04]

Yes, I agree with implementing max_size. Adding a way to cancel read directory
call is a big task. I'm going to add max_size to ReadDirectory call, and remove
ReadDirectoryEntryIds.

+    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.
 //
@@ -494,7 +546,7 @@ void MTPDeviceDelegateImplLinux::ReadBytes(
                                        closure));
 }
 
-bool MTPDeviceDelegateImplLinux::IsReadOnly() {
+bool MTPDeviceDelegateImplLinux::IsReadOnly() const {
   return read_only_;
 }
 
@@ -520,6 +572,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 +808,104 @@ 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
+    RunDeleteObjectOnUIThread(file_path, success_callback, error_callback);
+
+  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);
+  } else {
+    // Read directory entry ids to check the directory is empty.
+    uint32 directory_id;
+    if (CachedPathToId(file_path, &directory_id)) {
+      const MTPDeviceTaskHelper::ReadDirectoryEntryIdsSuccessCallback

[Lei Zhang, 2015/03/07 01:17:24]

Before we issue the ReadDirectory call, can we check the cache first? We'll need
to add a HasChildren() method to MTPDeviceDelegateImplLinux::MTPFileNode. If
HasChildren() return true, we know it's not empty, else we stil need to call
ReadDirectory, since the cache could just be empty.

[yawano, 2015/03/09 04:37:49]

Done.

+          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(
+          base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
+    } else {
+      error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+    }
+  }
+
+  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(

[Lei Zhang, 2015/03/07 01:17:24]

There's only one caller for this. I would just put get rid of this function and
put the code in DeleteFileInternal().

[yawano, 2015/03/09 04:37:49]

Done.

+    const base::FilePath& file_path,
+    const DeleteObjectSuccessCallback& success_callback,
+    const ErrorCallback& error_callback) {
+  uint32 object_id;
+  if (CachedPathToId(file_path, &object_id))
+    RunDeleteObjectOnUIThread(object_id, success_callback, error_callback);
+  else
+    error_callback.Run(base::File::FILE_ERROR_NOT_FOUND);
+}
+
+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);
+
+  EnsureInitAndRunTask(PendingTaskInfo(

[Lei Zhang, 2015/03/07 01:17:24]

You can just call content::BrowserThread::PostTask() directly at this point. The
first argument to PendingTaskInfo() is empty, and initialization must have
already taken place.

[yawano, 2015/03/09 04:37:48]

Done.

+      base::FilePath(), content::BrowserThread::UI, FROM_HERE, closure));
+}
+
 void MTPDeviceDelegateImplLinux::EnsureInitAndRunTask(
     const PendingTaskInfo& task_info) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
@@ -1022,20 +1212,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 +1311,17 @@ 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()) {
+    MTPFileNode* parent = it->second->parent();

[Lei Zhang, 2015/03/07 01:17:24]

Assuming we add MTPNode::HasChildren(), I would
DCHECK(!it->second->HasChildren()); here as well.

[yawano, 2015/03/09 04:37:48]

Done. Added also for DeleteChild.

+    if (parent) {
+      const bool ret = parent->DeleteChild(id);
+      DCHECK(ret);
+    }
+  }
+}
+
 void CreateMTPDeviceAsyncDelegate(
     const std::string& device_location,
     const bool read_only,