[Media Galleries] Switch Mac MTP delegate to async interface.

chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm

Index: chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm
diff --git a/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm b/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm
index 3cae2b590bd9bea23be88d79848e0f7af11ae3b3..29fedbce88e6320aedd82b0578c51f9c741fb4ad 100644
--- a/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm
+++ b/chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm
@@ -5,17 +5,29 @@
 #include "chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.h"
 
 #include "base/memory/scoped_nsobject.h"
-#include "base/sequenced_task_runner.h"
-#include "base/sequenced_task_runner_helpers.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/media_gallery/mtp_device_delegate_impl.h"
 #include "chrome/browser/storage_monitor/image_capture_device.h"
 #include "chrome/browser/storage_monitor/image_capture_device_manager.h"
 #include "chrome/browser/storage_monitor/media_storage_util.h"
 #include "content/public/browser/browser_thread.h"
+#include "webkit/fileapi/async_file_util.h"
 
 namespace chrome {
 
+namespace {
+
+using fileapi::MTPDeviceAsyncDelegate;
+typedef MTPDeviceAsyncDelegate::CreateSnapshotFileSuccessCallback
+    CreateSnapshotFileSuccessCallback;
+typedef MTPDeviceAsyncDelegate::ErrorCallback ErrorCallback;
+typedef MTPDeviceAsyncDelegate::GetFileInfoSuccessCallback
+    GetFileInfoSuccessCallback;
+typedef MTPDeviceAsyncDelegate::ReadDirectorySuccessCallback
+    ReadDirectorySuccessCallback;
+
+}  // namespace
+
 // This class handles the UI-thread hand-offs needed to interface
 // with the ImageCapture library. It will forward callbacks to
 // its delegate on the task runner with which it is created. All
@@ -42,6 +54,10 @@ class MTPDeviceDelegateImplMac::DeviceListener
                               base::PlatformFileError error) OVERRIDE;
   virtual void DeviceRemoved() OVERRIDE;
 
+  // Used during delegate destruction to ensure there are no more calls
+  // to the delegate by the listener.
+  virtual void ResetDelegate();
+
  private:
   scoped_nsobject<ImageCaptureDevice> camera_device_;
 
@@ -75,34 +91,38 @@ void MTPDeviceDelegateImplMac::DeviceListener::DownloadFile(
 void MTPDeviceDelegateImplMac::DeviceListener::ItemAdded(
     const std::string& name,
     const base::PlatformFileInfo& info) {
-  delegate_->ItemAdded(name, info);
+  if (delegate_)
+    delegate_->ItemAdded(name, info);
 }
 
 void MTPDeviceDelegateImplMac::DeviceListener::NoMoreItems() {
-  delegate_->NoMoreItems();
+  if (delegate_)
+    delegate_->NoMoreItems();
 }
 
 void MTPDeviceDelegateImplMac::DeviceListener::DownloadedFile(
     const std::string& name,
     base::PlatformFileError error) {
-delegate_->DownloadedFile(name, error);
+  if (delegate_)
+    delegate_->DownloadedFile(name, error);
 }
 
 void MTPDeviceDelegateImplMac::DeviceListener::DeviceRemoved() {
   [camera_device_ close];
   camera_device_.reset();
+  if (delegate_)
+    delegate_->NoMoreItems();
+}
+
+void MTPDeviceDelegateImplMac::DeviceListener::ResetDelegate() {
+  delegate_ = NULL;
 }
 
 MTPDeviceDelegateImplMac::MTPDeviceDelegateImplMac(
     const std::string& device_id,
-    const base::FilePath::StringType& synthetic_path,
-    base::SequencedTaskRunner* media_task_runner)
+    const base::FilePath::StringType& synthetic_path)
     : device_id_(device_id),
       root_path_(synthetic_path),
-      media_task_runner_(media_task_runner),
-      enumerator_(NULL),
-      file_download_event_(NULL),
-      file_download_error_(base::PLATFORM_FILE_OK),
       received_all_files_(false) {
 
   // Make a synthetic entry for the root of the filesystem.
@@ -118,99 +138,151 @@ MTPDeviceDelegateImplMac::MTPDeviceDelegateImplMac(
 }
 
 MTPDeviceDelegateImplMac::~MTPDeviceDelegateImplMac() {
-  DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
-  DCHECK(enumerator_ == NULL);
 }
 
-base::PlatformFileError MTPDeviceDelegateImplMac::GetFileInfo(
+namespace {
+
+void ForwardGetFileInfo(
+    base::PlatformFileInfo* info,
+    base::PlatformFileError* error,
+    const GetFileInfoSuccessCallback& success_callback,
+    const ErrorCallback& error_callback) {
+  scoped_ptr<base::PlatformFileInfo> info_deleter(info);
+  scoped_ptr<base::PlatformFileError> error_deleter(error);
+
+  if (*error == base::PLATFORM_FILE_OK) {
+    success_callback.Run(*info);
+  } else {
+    error_callback.Run(*error);
+  }
+}
+
+}  // namespace
+
+void MTPDeviceDelegateImplMac::GetFileInfo(
     const base::FilePath& file_path,
-    base::PlatformFileInfo* file_info) {
-  base::AutoLock lock(mutex_);
+    const GetFileInfoSuccessCallback& success_callback,
+    const ErrorCallback& error_callback) {
+  base::PlatformFileInfo* info = new base::PlatformFileInfo;
+  base::PlatformFileError* error = new base::PlatformFileError;
+  content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI,

[Lei Zhang, 2013/03/04 22:45:01]

You can do PostTaskAndReplyWithResults, such that GetFileInfoImpl returns a pair
with the info and error instead.

Alternatively, make |info| Owned(info) and similarly for |error| in the
ForwardGetFileInfo bind call, so ForwardGetFileInfo() does not need to have the
scoped deleters.

[Greg Billock, 2013/03/06 00:28:30]

Used Owned() here and in ReadDir.

+      FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::GetFileInfoImpl,
+                 base::Unretained(this), file_path, info, error),
+      base::Bind(&ForwardGetFileInfo,
+                 info, error, success_callback, error_callback));
+}
+
+void MTPDeviceDelegateImplMac::ReadDirectory(
+      const base::FilePath& root,
+      const ReadDirectorySuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::ReadDirectoryImpl,
+                 base::Unretained(this),
+                 root, success_callback, error_callback));
+}
+
+void MTPDeviceDelegateImplMac::CreateSnapshotFile(
+      const base::FilePath& device_file_path,
+      const base::FilePath& local_path,
+      const CreateSnapshotFileSuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::DownloadFile,
+                 base::Unretained(this),
+                 device_file_path, local_path,
+                 success_callback, error_callback));
+}
+
+void MTPDeviceDelegateImplMac::CancelPendingTasksAndDeleteDelegate() {
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::CancelAndDelete,
+                 base::Unretained(this)));
+}
+
+void MTPDeviceDelegateImplMac::GetFileInfoImpl(
+    const base::FilePath& file_path,
+    base::PlatformFileInfo* file_info,
+    base::PlatformFileError* error) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   base::hash_map<base::FilePath::StringType,
                  base::PlatformFileInfo>::const_iterator i =
       file_info_.find(file_path.value());
-  if (i == file_info_.end())
-    return base::PLATFORM_FILE_ERROR_NOT_FOUND;
-
+  if (i == file_info_.end()) {
+    *error = base::PLATFORM_FILE_ERROR_NOT_FOUND;
+    return;
+  }
   *file_info = i->second;
-  return base::PLATFORM_FILE_OK;
+  *error = base::PLATFORM_FILE_OK;
 }
 
-scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
-MTPDeviceDelegateImplMac::CreateFileEnumerator(const base::FilePath& root,
-                                               bool recursive) {
-  base::AutoLock lock(mutex_);
-  DCHECK(!enumerator_);
-  enumerator_ = new Enumerator(this);
-  return make_scoped_ptr(enumerator_)
-      .PassAs<fileapi::FileSystemFileUtil::AbstractFileEnumerator>();
+void MTPDeviceDelegateImplMac::ReadDirectoryImpl(
+      const base::FilePath& root,
+      const ReadDirectorySuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  read_directory_success_callback_ = success_callback;

[Lei Zhang, 2013/03/04 22:45:01]

Isn't it possible for this delegate to receive multiple read directory calls?
i.e. there can be more than 1 request in flight.

[Greg Billock, 2013/03/06 00:28:30]

I'm not sure. I'll ask.

+  read_directory_error_callback_ = error_callback;

[Lei Zhang, 2013/03/04 22:45:01]

|read_directory_error_callback_| never gets called.

[Greg Billock, 2013/03/06 00:28:30]

Yes. I'm not sure what to do about that. Schedule a timeout and call it if we
don't have metadata by that time?

+
+  if (received_all_files_)
+    NotifyReadDir();
+
+  // TODO(gbillock): Need to add a task on a timeout or something for
+  // the read dir failure?
 }
 
-base::PlatformFileError MTPDeviceDelegateImplMac::CreateSnapshotFile(
-    const base::FilePath& device_file_path,
-    const base::FilePath& local_path,
-    base::PlatformFileInfo* file_info) {
-  base::PlatformFileError error = GetFileInfo(device_file_path, file_info);
-  if (error != base::PLATFORM_FILE_OK)
-    return error;
-
-  // Set up to wait for download. Callers are ensuring this particular function
-  // will not be re-entered.
-  base::WaitableEvent waiter(true, false);
-  {
-    base::AutoLock lock(mutex_);
-    DCHECK(file_download_event_ == NULL);
-    file_download_event_ = &waiter;
-  }
+void MTPDeviceDelegateImplMac::DownloadFile(
+      const base::FilePath& device_file_path,
+      const base::FilePath& local_path,
+      const CreateSnapshotFileSuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  read_file_success_callback_ = success_callback;
+  read_file_error_callback_ = error_callback;
 
-  // Start the download in the UI thread.
-  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
-      base::Bind(&DeviceListener::DownloadFile,
-                 base::Unretained(camera_interface_.get()),
-                 device_file_path.BaseName().value(), local_path));
-  waiter.Wait();
-  {
-    base::AutoLock lock(mutex_);
-    file_download_event_ = NULL;
-    error = file_download_error_;
+  base::PlatformFileError error;
+  base::PlatformFileInfo info;
+  GetFileInfoImpl(device_file_path, &info, &error);
+  if (error != base::PLATFORM_FILE_OK) {
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+                                     base::Bind(read_file_error_callback_,
+                                                error));
+    read_file_success_callback_.Reset();
+    read_file_error_callback_.Reset();
+    return;
   }
 
-  // Modify the last modified time to null. This prevents the time stamp
-  // verification in LocalFileStreamReader.
-  file_info->last_modified = base::Time();
-
-  return error;
+  camera_interface_->DownloadFile(device_file_path.BaseName().value(),
+                                  local_path);
 }
 
-void MTPDeviceDelegateImplMac::CancelPendingTasksAndDeleteDelegate() {
+void MTPDeviceDelegateImplMac::CancelAndDelete() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   // Artificially pretend that we have already gotten all items we're going
   // to get.
   NoMoreItems();
 
-  {
-    base::AutoLock lock(mutex_);
-    // Artificially wake up any downloads pending with an error code.
-    if (file_download_event_) {
-      file_download_error_ = base::PLATFORM_FILE_ERROR_FAILED;
-      file_download_event_->Signal();
-    }
-  }
+  CancelDownloads();
 
   // Schedule the camera session to be closed and the interface deleted.
+  camera_interface_->ResetDelegate();
   content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
       base::Bind(&DeviceListener::CloseCameraSessionAndDelete,
                  base::Unretained(camera_interface_.release())));
 
-  media_task_runner_->DeleteSoon(FROM_HERE, this);
+  delete this;
+}
+
+void MTPDeviceDelegateImplMac::CancelDownloads() {
+  // TODO(gbillock): implement.
+  read_file_success_callback_.Reset();
+  read_file_error_callback_.Reset();
 }
 
+// Called on the UI thread by the listener
 void MTPDeviceDelegateImplMac::ItemAdded(
     const std::string& name, const base::PlatformFileInfo& info) {
-  base::AutoLock lock(mutex_);
-
-  // Make sure that if we're canceled and an enumerator is awake, that
-  // it will stay consistent. May need to revisit this if we need
-  // notifications of files added after we think we're done.
   if (received_all_files_)
     return;
 
@@ -224,95 +296,76 @@ void MTPDeviceDelegateImplMac::ItemAdded(
   file_info_[item_filename.value()] = info;
   file_paths_.push_back(item_filename);
 
-  if (enumerator_)
-    enumerator_->ItemsChanged();
+  // TODO(gbillock): Should we send new files to
+  // read_directory_success_callback?
 }
 
+// Called in the UI thread by delegate.
 void MTPDeviceDelegateImplMac::NoMoreItems() {
-  base::AutoLock lock(mutex_);
   received_all_files_ = true;
-
-  if (enumerator_)
-    enumerator_->ItemsChanged();
-}
-
-void MTPDeviceDelegateImplMac::DownloadedFile(
-    const std::string& name, base::PlatformFileError error) {
-  // If we're cancelled and deleting, we have already signaled all enumerators.
-  if (!camera_interface_.get())
-    return;
-
-  base::AutoLock lock(mutex_);
-  file_download_error_ = error;
-  file_download_event_->Signal();
-}
-
-base::FilePath MTPDeviceDelegateImplMac::GetFile(size_t index) {
-  base::AutoLock lock(mutex_);
-  if (index >= file_paths_.size())
-    return base::FilePath();
-  else
-    return file_paths_[index];
-}
-
-bool MTPDeviceDelegateImplMac::ReceivedAllFiles() {
-  base::AutoLock lock(mutex_);
-  return received_all_files_;
+  NotifyReadDir();
 }
 
-void MTPDeviceDelegateImplMac::RemoveEnumerator(Enumerator* enumerator) {
-  base::AutoLock lock(mutex_);
-  DCHECK(enumerator_ == enumerator);
-  enumerator_ = NULL;
+void ForwardReadDir(
+    const ReadDirectorySuccessCallback& success_callback,
+    fileapi::AsyncFileUtil::EntryList* entry_list,
+    bool has_more) {
+  scoped_ptr<fileapi::AsyncFileUtil::EntryList> list_deleter(entry_list);
+  success_callback.Run(*entry_list, has_more);
 }
 
-MTPDeviceDelegateImplMac::Enumerator::Enumerator(
-    MTPDeviceDelegateImplMac* delegate)
-    : delegate_(delegate),
-      position_(0),
-      wait_for_items_(false, false) {}
-
-MTPDeviceDelegateImplMac::Enumerator::~Enumerator() {
-  delegate_->RemoveEnumerator(this);
-}
+void MTPDeviceDelegateImplMac::NotifyReadDir() {
+  if (!read_directory_success_callback_.is_null()) {
+    // TODO(gbillock): should we just switch to using this directly internally
+    // instead of PlatformFileInfo?
+    fileapi::AsyncFileUtil::EntryList* entry_list =
+        new fileapi::AsyncFileUtil::EntryList;
+    for (size_t i = 0; i < file_paths_.size(); ++i) {
+      base::PlatformFileInfo info = file_info_[file_paths_[i].value()];
+      base::FileUtilProxy::Entry entry;
+      entry.name = file_paths_[i].value();
+      entry.is_directory = info.is_directory;
+      entry.size = info.size;
+      entry.last_modified_time = info.last_modified;
+      entry_list->push_back(entry);
+    }
+    content::BrowserThread::PostTask(content::BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&ForwardReadDir,
+                   read_directory_success_callback_, entry_list, false));
 
-base::FilePath MTPDeviceDelegateImplMac::Enumerator::Next() {
-  base::FilePath next_file = delegate_->GetFile(position_);
-  while (next_file.empty() && !delegate_->ReceivedAllFiles()) {
-    wait_for_items_.Wait();
-    next_file = delegate_->GetFile(position_);
+    read_directory_success_callback_.Reset();
+    read_directory_error_callback_.Reset();
   }
-
-  position_++;
-  return next_file;
-}
-
-int64 MTPDeviceDelegateImplMac::Enumerator::Size() {
-  base::PlatformFileInfo info;
-  delegate_->GetFileInfo(delegate_->GetFile(position_ - 1), &info);
-  return info.size;
-}
-
-base::Time MTPDeviceDelegateImplMac::Enumerator::LastModifiedTime() {
-  base::PlatformFileInfo info;
-  delegate_->GetFileInfo(delegate_->GetFile(position_ - 1), &info);
-  return info.last_modified;
 }
 
-bool MTPDeviceDelegateImplMac::Enumerator::IsDirectory() {
-  base::PlatformFileInfo info;
-  delegate_->GetFileInfo(delegate_->GetFile(position_ - 1), &info);
-  return info.is_directory;
-}
+// Invoked on UI thread from the listener.
+void MTPDeviceDelegateImplMac::DownloadedFile(
+    const std::string& name, base::PlatformFileError error) {
+  // If we're cancelled and deleting, we may have deleted the camera.
+  if (!camera_interface_.get())
+    return;
 
-void MTPDeviceDelegateImplMac::Enumerator::ItemsChanged() {
-  wait_for_items_.Signal();
+  if (!read_file_success_callback_.is_null()) {
+    if (error == base::PLATFORM_FILE_OK) {
+      base::FilePath path(name);
+      base::PlatformFileInfo info = file_info_[path.value()];
+      content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+          base::Bind(read_file_success_callback_, info, path));
+    } else {
+      content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+          base::Bind(read_file_error_callback_, error));
+    }
+    read_file_success_callback_.Reset();
+    read_file_error_callback_.Reset();
+  }
 }
 
-void CreateMTPDeviceDelegate(const std::string& device_location,
-                             base::SequencedTaskRunner* media_task_runner,
-                             const CreateMTPDeviceDelegateCallback& cb) {
-  std::string device_name = base::FilePath(device_location).BaseName().value();
+void CreateMTPDeviceAsyncDelegate(
+    const base::FilePath::StringType& device_location,
+    const CreateMTPDeviceAsyncDelegateCallback& cb) {
+  std::string device_name =
+      base::FilePath(device_location).BaseName().value();
   std::string device_id;
   MediaStorageUtil::Type type;
   bool cracked = MediaStorageUtil::CrackDeviceId(device_name,
@@ -320,8 +373,7 @@ void CreateMTPDeviceDelegate(const std::string& device_location,
   DCHECK(cracked);
   DCHECK_EQ(MediaStorageUtil::MAC_IMAGE_CAPTURE, type);
 
-  cb.Run(new MTPDeviceDelegateImplMac(device_id, device_location,
-                                      media_task_runner));
+  cb.Run(new MTPDeviceDelegateImplMac(device_id, device_location));
 }
 
 }  // namespace chrome