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

BUG=None


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

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

chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.h

@@ -5,21 +5,15 @@
 #ifndef CHROME_BROWSER_MEDIA_GALLERIES_MTP_DEVICE_DELEGATE_IMPL_MAC_H_
 #define CHROME_BROWSER_MEDIA_GALLERIES_MTP_DEVICE_DELEGATE_IMPL_MAC_H_
 
+#include <list>
+#include <map>
 #include <vector>
 
 #include "base/files/file_path.h"
 #include "base/hash_tables.h"
 #include "base/memory/weak_ptr.h"
 #include "base/platform_file.h"
-#include "base/sequenced_task_runner_helpers.h"
-#include "base/synchronization/lock.h"
-#include "base/synchronization/waitable_event.h"
-#include "webkit/fileapi/file_system_file_util.h"
-#include "webkit/fileapi/media/mtp_device_delegate.h"
-
-namespace base {
-class SequencedTaskRunner;
-}
+#include "webkit/fileapi/media/mtp_device_async_delegate.h"
 
 namespace chrome {
 
@@ -28,94 +22,89 @@ namespace chrome {
 // and names of all files notified through the ItemAdded call will be
 // all appear as children of that directory. (ItemAdded calls with directories
 // will be ignored.)
-class MTPDeviceDelegateImplMac : public fileapi::MTPDeviceDelegate {
+// Note on thread management: This class is thread-compatible: it can be created
+// on any thread, but then mutates all state on the UI thread. The async
+// delegate interface can be invoked on any thread, as it simply forwards calls
+// to the UI thread.
+class MTPDeviceDelegateImplMac : public fileapi::MTPDeviceAsyncDelegate {
  public:
   MTPDeviceDelegateImplMac(const std::string& device_id,
-                           const base::FilePath::StringType& synthetic_path,
-                           base::SequencedTaskRunner* media_task_runner);
+                           const base::FilePath::StringType& synthetic_path);
 
-  // MTPDeviceDelegate:
-  virtual base::PlatformFileError GetFileInfo(
+  // MTPDeviceAsyncDelegate implementation. These functions are called on the
+  // IO thread by the async filesystem file util. They forward to
+  // similarly-named methods on the UI thread.
+  virtual void GetFileInfo(
       const base::FilePath& file_path,
-      base::PlatformFileInfo* file_info) OVERRIDE;
-  virtual scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
-      CreateFileEnumerator(const base::FilePath& root, bool recursive) OVERRIDE;
-  virtual base::PlatformFileError CreateSnapshotFile(
+      const GetFileInfoSuccessCallback& success_callback,
+      const ErrorCallback& error_callback) OVERRIDE;
+  virtual void ReadDirectory(
+      const base::FilePath& root,
+      const ReadDirectorySuccessCallback& success_callback,
+      const ErrorCallback& error_callback) OVERRIDE;
+  virtual void CreateSnapshotFile(
       const base::FilePath& device_file_path,
       const base::FilePath& local_path,
-      base::PlatformFileInfo* file_info) OVERRIDE;
+      const CreateSnapshotFileSuccessCallback& success_callback,
+      const ErrorCallback& error_callback) OVERRIDE;
   virtual void CancelPendingTasksAndDeleteDelegate() OVERRIDE;
 
-  // Forward delegates for ImageCaptureDeviceListener
+  // Forward delegates for ImageCaptureDeviceListener. These are
+  // invoked in callbacks of the ImageCapture library on the UI thread.
   virtual void ItemAdded(const std::string& name,
                          const base::PlatformFileInfo& info);
   virtual void NoMoreItems();
   virtual void DownloadedFile(const std::string& name,
                               base::PlatformFileError error);
 
-  // Implementation returned by |CreateFileEnumerator()|. Must be deleted
-  // before CancelPendingTasksAndDeleteDelegate is called.
-  class Enumerator :
-      public fileapi::FileSystemFileUtil::AbstractFileEnumerator {
-   public:
-    explicit Enumerator(MTPDeviceDelegateImplMac* delegate);
-    virtual ~Enumerator();
+  // Scheduled when early directory reads are requested. The
+  // timeout will signal an ABORT error to the caller if the
+  // device metadata cannot be read.
+  void ReadDirectoryTimeout(const base::FilePath& root);
 
-    virtual base::FilePath Next() OVERRIDE;
-    virtual int64 Size() OVERRIDE;
-    virtual base::Time LastModifiedTime() OVERRIDE;
-    virtual bool IsDirectory() OVERRIDE;
+ private:
+  class DeviceListener;
 
-    // Called by the delegate to signal any waiters that the received items
-    // list has changed state.
-    void ItemsChanged();
+  virtual ~MTPDeviceDelegateImplMac();
 
-   private:
-    MTPDeviceDelegateImplMac* delegate_;
-    size_t position_;
-    base::WaitableEvent wait_for_items_;
-  };
+  // Delegate for GetFileInfo, called on the UI thread.
+  void GetFileInfoImpl(const base::FilePath& file_path,
+                       base::PlatformFileInfo* file_info,
+                       base::PlatformFileError* error);
 
-  // GetFile and HasAllFiles called by enumerators.
-  base::FilePath GetFile(size_t index);
-  bool ReceivedAllFiles();
-  void RemoveEnumerator(Enumerator* enumerator);
+  // Delegate for ReadDirectory, called on the UI thread.
+  void ReadDirectoryImpl(
+      const base::FilePath& root,
+      const ReadDirectorySuccessCallback& success_callback,
+      const ErrorCallback& error_callback);
 
- private:
-  friend class base::DeleteHelper<MTPDeviceDelegateImplMac>;
+  // Delegate for CreateSnapshotFile, called on the UI thread.
+  void DownloadFile(
+      const base::FilePath& device_file_path,
+      const base::FilePath& local_path,
+      const CreateSnapshotFileSuccessCallback& success_callback,
+      const ErrorCallback& error_callback);
 
-  class DeviceListener;
+  // Public for closures; should not be called except by
+  // CancelTasksAndDeleteDelegate.
+  void CancelAndDelete();
 
-  virtual ~MTPDeviceDelegateImplMac();
+  // Cancels any outstanding downloads.
+  void CancelDownloads();
+
+  // If necessary, notifies the ReadDirectory callback that all data
+  // has been read.
+  void NotifyReadDir();
 
   std::string device_id_;
   base::FilePath root_path_;
 
-  // Stores a reference to worker pool thread. All requests and response of file
-  // operations are posted on |media_task_runner_|. All callbacks from the
-  // camera will come through this task runner as well.
-  scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
-
   // Interface object for the camera underlying this MTP session.
   scoped_ptr<DeviceListener> camera_interface_;
 
-  // This lock protects all subsequent state. While calling into the delegate
-  // from the FileSystem API happens in sequence, these calls may be
-  // interleaved with calls on other threads in the sequenced task runner
-  // forwarded from the device.
-  base::Lock mutex_;
-
-  // Weak pointer to the enumerator which may be listening for files to come in.
-  Enumerator* enumerator_;
-
   // Stores a map from filename to file metadata received from the camera.
-  base::hash_map<base::FilePath::StringType, base::PlatformFileInfo> file_info_;
-
-  // Notification for pending download.
-  base::WaitableEvent* file_download_event_;
-
-  // Resulting error code for pending download.
-  base::PlatformFileError file_download_error_;
+  base::hash_map<base::FilePath::StringType,
+                 base::PlatformFileInfo> file_info_;
 
   // List of files received from the camera.
   std::vector<base::FilePath> file_paths_;
@@ -123,6 +112,28 @@ class MTPDeviceDelegateImplMac : public fileapi::MTPDeviceDelegate {
   // Set to true when all file metadata has been received from the camera.
   bool received_all_files_;
 
+  typedef std::map<std::string,
+                   std::pair<CreateSnapshotFileSuccessCallback,
+                             ErrorCallback> > ReadFileTransactionMap;
+
+  struct ReadDirectoryRequest {
+    ReadDirectoryRequest(const base::FilePath& dir,
+                         ReadDirectorySuccessCallback success_cb,
+                         ErrorCallback error_cb);
+    ~ReadDirectoryRequest();
+
+    base::FilePath directory;
+    ReadDirectorySuccessCallback success_callback;
+    ErrorCallback error_callback;
+  };
+
+  typedef std::list<ReadDirectoryRequest> ReadDirTransactionList;
+
+  ReadFileTransactionMap read_file_transactions_;
+  ReadDirTransactionList read_dir_transactions_;
+
+  base::WeakPtrFactory<MTPDeviceDelegateImplMac> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplMac);
 };
 

chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac.mm

@@ -5,17 +5,31 @@
 #include "chrome/browser/media_galleries/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_galleries/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 {
+
+int kReadDirectoryTimeLimitSeconds = 20;
+
+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 +56,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,35 +93,40 @@ void MTPDeviceDelegateImplMac::DeviceListener::DownloadFile(
 void MTPDeviceDelegateImplMac::DeviceListener::ItemAdded(
     const std::string& name,
     const base::PlatformFileInfo& info) {
+  if (delegate_)
     delegate_->ItemAdded(name, info);
 }
 
 void MTPDeviceDelegateImplMac::DeviceListener::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) {
+      received_all_files_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 
   // Make a synthetic entry for the root of the filesystem.
   base::PlatformFileInfo info;
@@ -118,99 +141,184 @@ 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) {
+  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;
+  // Note: ownership of these objects passed into the reply callback.
+  content::BrowserThread::PostTaskAndReply(content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::GetFileInfoImpl,
+                 base::Unretained(this), file_path, info, error),
+      base::Bind(&ForwardGetFileInfo,
+                 base::Owned(info), base::Owned(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_dir_transactions_.push_back(ReadDirectoryRequest(
+      root, success_callback, error_callback));
+
+  if (received_all_files_) {
+    NotifyReadDir();
+    return;
+  }
+
+  // Schedule a timeout in case the directory read doesn't complete.
+  content::BrowserThread::PostDelayedTask(
+      content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::ReadDirectoryTimeout,
+                 weak_factory_.GetWeakPtr(), root),
+      base::TimeDelta::FromSeconds(kReadDirectoryTimeLimitSeconds));
+}
+
+void MTPDeviceDelegateImplMac::ReadDirectoryTimeout(
+    const base::FilePath& root) {
+  if (received_all_files_)
+    return;
+
+  for (ReadDirTransactionList::iterator iter = read_dir_transactions_.begin();
+       iter != read_dir_transactions_.end();) {
+    if (iter->directory != root) {
+      iter++;
+      continue;
+    }
+    iter->error_callback.Run(base::PLATFORM_FILE_ERROR_ABORT);
+    iter = read_dir_transactions_.erase(iter);
+  }
 }
 
-base::PlatformFileError MTPDeviceDelegateImplMac::CreateSnapshotFile(
+void MTPDeviceDelegateImplMac::DownloadFile(
       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;
-  }
+      const CreateSnapshotFileSuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-  // 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(error_callback,
+                                                error));
+    return;
   }
 
-  // Modify the last modified time to null. This prevents the time stamp
-  // verification in LocalFileStreamReader.
-  file_info->last_modified = base::Time();
-
-  return error;
+  read_file_transactions_[device_file_path.BaseName().value()] =
+      std::make_pair(success_callback, error_callback);
+  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() {
+  // Cancel any outstanding callbacks.
+  for (ReadFileTransactionMap::iterator iter = read_file_transactions_.begin();
+       iter != read_file_transactions_.end(); ++iter) {
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+        base::Bind(iter->second.second, base::PLATFORM_FILE_ERROR_ABORT));
+  }
+  read_file_transactions_.clear();
+
+  for (ReadDirTransactionList::iterator iter = read_dir_transactions_.begin();
+       iter != read_dir_transactions_.end(); ++iter) {
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+        base::Bind(iter->error_callback, base::PLATFORM_FILE_ERROR_ABORT));
+  }
+  read_dir_transactions_.clear();
+
+  // TODO(gbillock): ImageCapture currently offers no way to cancel
+  // in-progress downloads.
+}
+
+// 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,94 +332,74 @@ 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_dir_transactions_ callbacks?
 }
 
+// Called in the UI thread by delegate.
 void MTPDeviceDelegateImplMac::NoMoreItems() {
-  base::AutoLock lock(mutex_);
   received_all_files_ = true;
+  NotifyReadDir();
+}
 
-  if (enumerator_)
-    enumerator_->ItemsChanged();
+void MTPDeviceDelegateImplMac::NotifyReadDir() {
+  // Note: this assumes the only directory read we get is for the root.
+  // When this class supports directory hierarchies, this will change.
+  for (ReadDirTransactionList::iterator iter = read_dir_transactions_.begin();
+       iter != read_dir_transactions_.end(); ++iter) {
+    fileapi::AsyncFileUtil::EntryList entry_list;
+    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(iter->success_callback, entry_list, false));
+  }
 }
 
+// Invoked on UI thread from the listener.
 void MTPDeviceDelegateImplMac::DownloadedFile(
     const std::string& name, base::PlatformFileError error) {
-  // If we're cancelled and deleting, we have already signaled all enumerators.
+  // If we're cancelled and deleting, we may have deleted the camera.
   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_;
-}
-
-void MTPDeviceDelegateImplMac::RemoveEnumerator(Enumerator* enumerator) {
-  base::AutoLock lock(mutex_);
-  DCHECK(enumerator_ == enumerator);
-  enumerator_ = NULL;
-}
-
-MTPDeviceDelegateImplMac::Enumerator::Enumerator(
-    MTPDeviceDelegateImplMac* delegate)
-    : delegate_(delegate),
-      position_(0),
-      wait_for_items_(false, false) {}
-
-MTPDeviceDelegateImplMac::Enumerator::~Enumerator() {
-  delegate_->RemoveEnumerator(this);
-}
+  ReadFileTransactionMap::iterator iter = read_file_transactions_.find(name);
+  if (iter == read_file_transactions_.end())
+    return;
 
-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_);
+  if (error != base::PLATFORM_FILE_OK) {
+    content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+        base::Bind(iter->second.second, error));
+    read_file_transactions_.erase(iter);
+    return;
   }
 
-  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;
+  base::PlatformFileInfo info = file_info_[name];
+  content::BrowserThread::PostTask(content::BrowserThread::IO, FROM_HERE,
+      base::Bind(iter->second.first, info, base::FilePath(name)));
+  read_file_transactions_.erase(iter);
 }
 
-bool MTPDeviceDelegateImplMac::Enumerator::IsDirectory() {
-  base::PlatformFileInfo info;
-  delegate_->GetFileInfo(delegate_->GetFile(position_ - 1), &info);
-  return info.is_directory;
-}
+MTPDeviceDelegateImplMac::ReadDirectoryRequest::ReadDirectoryRequest(
+    const base::FilePath& dir,
+    ReadDirectorySuccessCallback success_cb,
+    ErrorCallback error_cb)
+    : directory(dir),
+      success_callback(success_cb),
+      error_callback(error_cb) {}
 
-void MTPDeviceDelegateImplMac::Enumerator::ItemsChanged() {
-  wait_for_items_.Signal();
-}
+MTPDeviceDelegateImplMac::ReadDirectoryRequest::~ReadDirectoryRequest() {}
 
-void CreateMTPDeviceDelegate(const std::string& device_location,
-                             base::SequencedTaskRunner* media_task_runner,
-                             const CreateMTPDeviceDelegateCallback& cb) {
+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;
@@ -320,8 +408,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

chrome/browser/media_galleries/mac/mtp_device_delegate_impl_mac_unittest.mm

@@ -6,10 +6,12 @@
 #import <ImageCaptureCore/ImageCaptureCore.h>
 
 #include "base/file_util.h"
+#include "base/files/scoped_temp_dir.h"
 #include "base/mac/cocoa_protocols.h"
 #include "base/mac/foundation_util.h"
 #include "base/memory/scoped_nsobject.h"
 #include "base/message_loop.h"
+#include "base/run_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/test/sequenced_worker_pool_owner.h"
@@ -38,6 +40,7 @@
 namespace {
 
 const char kDeviceId[] = "id";
+const char kDevicePath[] = "/ic:id";
 const char kTestFileContents[] = "test";
 
 }  // namespace
@@ -175,6 +178,11 @@ class MTPDeviceDelegateImplMacTest : public testing::Test {
   virtual void SetUp() OVERRIDE {
     ui_thread_.reset(new content::TestBrowserThread(
         content::BrowserThread::UI, &message_loop_));
+    file_thread_.reset(new content::TestBrowserThread(
+        content::BrowserThread::FILE, &message_loop_));
+    io_thread_.reset(new content::TestBrowserThread(
+        content::BrowserThread::IO));
+    ASSERT_TRUE(io_thread_->Start());
 
     manager_.SetNotifications(monitor_.receiver());
 
@@ -182,34 +190,134 @@ class MTPDeviceDelegateImplMacTest : public testing::Test {
     id<ICDeviceBrowserDelegate> delegate = manager_.device_browser();
     [delegate deviceBrowser:nil didAddDevice:camera_ moreComing:NO];
 
-    base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
-    task_runner_ = pool->GetSequencedTaskRunner(
-        pool->GetNamedSequenceToken("token-name"));
-    delegate_ = new chrome::MTPDeviceDelegateImplMac(
-        "id", "/ic:id", task_runner_.get());
+    delegate_ = new chrome::MTPDeviceDelegateImplMac(kDeviceId, kDevicePath);
+  }
+
+  void OnError(base::WaitableEvent* event, base::PlatformFileError error) {
+    error_ = error;
+    event->Signal();
+  }
+
+  void OverlappedOnError(base::WaitableEvent* event,
+                         base::PlatformFileError error) {
+    overlapped_error_ = error;
+    event->Signal();
+  }
+
+  void OnFileInfo(base::WaitableEvent* event,
+                  const base::PlatformFileInfo& info) {
+    error_ = base::PLATFORM_FILE_OK;
+    info_ = info;
+    event->Signal();
+  }
+
+  void OnReadDir(base::WaitableEvent* event,
+                 const fileapi::AsyncFileUtil::EntryList& files,
+                 bool has_more) {
+    error_ = base::PLATFORM_FILE_OK;
+    ASSERT_FALSE(has_more);
+    file_list_ = files;
+    event->Signal();
+  }
+
+  void OverlappedOnReadDir(base::WaitableEvent* event,
+                           const fileapi::AsyncFileUtil::EntryList& files,
+                           bool has_more) {
+    overlapped_error_ = base::PLATFORM_FILE_OK;
+    ASSERT_FALSE(has_more);
+    overlapped_file_list_ = files;
+    event->Signal();
+  }
+
+  void OnDownload(base::WaitableEvent* event,
+                  const base::PlatformFileInfo& file_info,
+                  const base::FilePath& local_path) {
+    error_ = base::PLATFORM_FILE_OK;
+    event->Signal();
+  }
+
+  base::PlatformFileError GetFileInfo(const base::FilePath& path,
+                                      base::PlatformFileInfo* info) {
+    base::WaitableEvent wait(true, false);
+    delegate_->GetFileInfo(
+      path,
+      base::Bind(&MTPDeviceDelegateImplMacTest::OnFileInfo,
+                 base::Unretained(this),
+                 &wait),
+      base::Bind(&MTPDeviceDelegateImplMacTest::OnError,
+                 base::Unretained(this),
+                 &wait));
+    base::RunLoop loop;
+    loop.RunUntilIdle();
+    EXPECT_TRUE(wait.IsSignaled());
+    *info = info_;
+    return error_;
+  }
+
+  base::PlatformFileError ReadDir(const base::FilePath& path) {
+    base::WaitableEvent wait(true, false);
+    delegate_->ReadDirectory(
+        path,
+        base::Bind(&MTPDeviceDelegateImplMacTest::OnReadDir,
+                   base::Unretained(this),
+                   &wait),
+        base::Bind(&MTPDeviceDelegateImplMacTest::OnError,
+                   base::Unretained(this),
+                   &wait));
+    base::RunLoop loop;
+    loop.RunUntilIdle();
+    wait.Wait();
+    return error_;
+  }
+
+  base::PlatformFileError DownloadFile(
+      const base::FilePath& path,
+      const base::FilePath& local_path) {
+    base::WaitableEvent wait(true, false);
+    delegate_->CreateSnapshotFile(
+        path, local_path,
+        base::Bind(&MTPDeviceDelegateImplMacTest::OnDownload,
+                   base::Unretained(this),
+                   &wait),
+        base::Bind(&MTPDeviceDelegateImplMacTest::OnError,
+                   base::Unretained(this),
+                   &wait));
+    base::RunLoop loop;
+    loop.RunUntilIdle();
+    wait.Wait();
+    return error_;
   }
 
   virtual void TearDown() OVERRIDE {
     id<ICDeviceBrowserDelegate> delegate = manager_.device_browser();
     [delegate deviceBrowser:nil didRemoveDevice:camera_ moreGoing:NO];
 
-    task_runner_->PostTask(FROM_HERE,
-        base::Bind(&chrome::MTPDeviceDelegateImplMac::
-                        CancelPendingTasksAndDeleteDelegate,
-                   base::Unretained(delegate_)));
+    delegate_->CancelPendingTasksAndDeleteDelegate();
+
+    io_thread_->Stop();
   }
 
  protected:
   MessageLoopForUI message_loop_;
+  // Note: threads must be made in this order: UI > FILE > IO
   scoped_ptr<content::TestBrowserThread> ui_thread_;
+  scoped_ptr<content::TestBrowserThread> file_thread_;
+  scoped_ptr<content::TestBrowserThread> io_thread_;
+  base::ScopedTempDir temp_dir_;
   chrome::test::TestStorageMonitor monitor_;
   chrome::ImageCaptureDeviceManager manager_;
-  ICCameraDevice* camera_;
-  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  MockMTPICCameraDevice* camera_;
 
   // This object needs special deletion inside the above |task_runner_|.
   chrome::MTPDeviceDelegateImplMac* delegate_;
 
+  base::PlatformFileError error_;
+  base::PlatformFileInfo info_;
+  fileapi::AsyncFileUtil::EntryList file_list_;
+
+  base::PlatformFileError overlapped_error_;
+  fileapi::AsyncFileUtil::EntryList overlapped_file_list_;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplMacTest);
 };
@@ -219,17 +327,61 @@ TEST_F(MTPDeviceDelegateImplMacTest, TestGetRootFileInfo) {
   // Making a fresh delegate should have a single file entry for the synthetic
   // root directory, with the name equal to the device id string.
   EXPECT_EQ(base::PLATFORM_FILE_OK,
-            delegate_->GetFileInfo(base::FilePath("/ic:id"), &info));
+            GetFileInfo(base::FilePath(kDevicePath), &info));
   EXPECT_TRUE(info.is_directory);
   EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND,
-            delegate_->GetFileInfo(base::FilePath("/nonexistent"), &info));
+            GetFileInfo(base::FilePath("/nonexistent"), &info));
 
   // Signal the delegate that no files are coming.
   delegate_->NoMoreItems();
 
-  scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator> enumerator =
-      delegate_->CreateFileEnumerator(base::FilePath("/ic:id"), true);
-  EXPECT_TRUE(enumerator->Next().empty());
+  EXPECT_EQ(base::PLATFORM_FILE_OK, ReadDir(base::FilePath(kDevicePath)));
+  EXPECT_EQ(0U, file_list_.size());
+}
+
+TEST_F(MTPDeviceDelegateImplMacTest, TestOverlappedReadDir) {
+  base::Time time1 = base::Time::Now();
+  base::PlatformFileInfo info1;
+  info1.size = 1;
+  info1.is_directory = false;
+  info1.is_symbolic_link = false;
+  info1.last_modified = time1;
+  info1.last_accessed = time1;
+  info1.creation_time = time1;
+  delegate_->ItemAdded("name1", info1);
+
+  base::WaitableEvent wait(true, false);
+
+  delegate_->ReadDirectory(
+      base::FilePath(kDevicePath),
+      base::Bind(&MTPDeviceDelegateImplMacTest::OnReadDir,
+                 base::Unretained(this),
+                 &wait),
+      base::Bind(&MTPDeviceDelegateImplMacTest::OnError,
+                 base::Unretained(this),
+                 &wait));
+
+  delegate_->ReadDirectory(
+      base::FilePath(kDevicePath),
+      base::Bind(&MTPDeviceDelegateImplMacTest::OverlappedOnReadDir,
+                 base::Unretained(this),
+                 &wait),
+      base::Bind(&MTPDeviceDelegateImplMacTest::OverlappedOnError,
+                 base::Unretained(this),
+                 &wait));
+
+
+  // Signal the delegate that no files are coming.
+  delegate_->NoMoreItems();
+
+  base::RunLoop loop;
+  loop.RunUntilIdle();
+  wait.Wait();
+
+  EXPECT_EQ(base::PLATFORM_FILE_OK, error_);
+  EXPECT_EQ(1U, file_list_.size());
+  EXPECT_EQ(base::PLATFORM_FILE_OK, overlapped_error_);
+  EXPECT_EQ(1U, overlapped_file_list_.size());
 }
 
 TEST_F(MTPDeviceDelegateImplMacTest, TestGetFileInfo) {
@@ -245,7 +397,7 @@ TEST_F(MTPDeviceDelegateImplMacTest, TestGetFileInfo) {
 
   base::PlatformFileInfo info;
   EXPECT_EQ(base::PLATFORM_FILE_OK,
-            delegate_->GetFileInfo(base::FilePath("/ic:id/name1"), &info));
+            GetFileInfo(base::FilePath("/ic:id/name1"), &info));
   EXPECT_EQ(info1.size, info.size);
   EXPECT_EQ(info1.is_directory, info.is_directory);
   EXPECT_EQ(info1.last_modified, info.last_modified);
@@ -257,27 +409,19 @@ TEST_F(MTPDeviceDelegateImplMacTest, TestGetFileInfo) {
   delegate_->NoMoreItems();
 
   EXPECT_EQ(base::PLATFORM_FILE_OK,
-            delegate_->GetFileInfo(base::FilePath("/ic:id/name2"), &info));
+            GetFileInfo(base::FilePath("/ic:id/name2"), &info));
   EXPECT_EQ(info1.size, info.size);
 
-  scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator> enumerator =
-      delegate_->CreateFileEnumerator(base::FilePath("/ic:id"), true);
-  base::FilePath next = enumerator->Next();
-  ASSERT_FALSE(next.empty());
-  EXPECT_EQ(1, enumerator->Size());
-  EXPECT_EQ(time1, enumerator->LastModifiedTime());
-  EXPECT_FALSE(enumerator->IsDirectory());
-  EXPECT_EQ("/ic:id/name1", next.value());
-
-  next = enumerator->Next();
-  ASSERT_FALSE(next.empty());
-  EXPECT_EQ(2, enumerator->Size());
-  EXPECT_EQ(time1, enumerator->LastModifiedTime());
-  EXPECT_FALSE(enumerator->IsDirectory());
-  EXPECT_EQ("/ic:id/name2", next.value());
-
-  next = enumerator->Next();
-  EXPECT_TRUE(next.empty());
+  EXPECT_EQ(base::PLATFORM_FILE_OK, ReadDir(base::FilePath(kDevicePath)));
+
+  ASSERT_EQ(2U, file_list_.size());
+  EXPECT_EQ(time1, file_list_[0].last_modified_time);
+  EXPECT_FALSE(file_list_[0].is_directory);
+  EXPECT_EQ("/ic:id/name1", file_list_[0].name);
+
+  EXPECT_EQ(time1, file_list_[1].last_modified_time);
+  EXPECT_FALSE(file_list_[1].is_directory);
+  EXPECT_EQ("/ic:id/name2", file_list_[1].name);
 }
 
 TEST_F(MTPDeviceDelegateImplMacTest, TestIgnoreDirectories) {
@@ -299,54 +443,49 @@ TEST_F(MTPDeviceDelegateImplMacTest, TestIgnoreDirectories) {
   delegate_->ItemAdded("name2", info1);
   delegate_->NoMoreItems();
 
-  scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator> enumerator =
-      delegate_->CreateFileEnumerator(base::FilePath("/ic:id"), true);
-  base::FilePath next = enumerator->Next();
-  ASSERT_FALSE(next.empty());
-  EXPECT_EQ("/ic:id/name1", next.value());
+  EXPECT_EQ(base::PLATFORM_FILE_OK, ReadDir(base::FilePath(kDevicePath)));
 
-  next = enumerator->Next();
-  ASSERT_FALSE(next.empty());
-  EXPECT_EQ("/ic:id/name2", next.value());
+  ASSERT_EQ(2U, file_list_.size());
+  EXPECT_EQ(time1, file_list_[0].last_modified_time);
+  EXPECT_FALSE(file_list_[0].is_directory);
+  EXPECT_EQ("/ic:id/name1", file_list_[0].name);
 
-  next = enumerator->Next();
-  EXPECT_TRUE(next.empty());
+  EXPECT_EQ(time1, file_list_[1].last_modified_time);
+  EXPECT_FALSE(file_list_[1].is_directory);
+  EXPECT_EQ("/ic:id/name2", file_list_[1].name);
 }
 
-TEST_F(MTPDeviceDelegateImplMacTest, EnumeratorWaitsForEntries) {
-  base::Time time1 = base::Time::Now();
-  base::PlatformFileInfo info1;
-  info1.size = 1;
-  info1.is_directory = false;
-  info1.is_symbolic_link = false;
-  info1.last_modified = time1;
-  info1.last_accessed = time1;
-  info1.creation_time = time1;
-  delegate_->ItemAdded("name1", info1);
-
-  scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator> enumerator =
-      delegate_->CreateFileEnumerator(base::FilePath("/ic:id"), true);
-  // Event is manually reset, initially unsignaled
-  base::WaitableEvent event(true, false);
-  base::FilePath next;
-  task_runner_->PostTask(FROM_HERE,
-                         base::Bind(&EnumerateAndSignal,
-                         enumerator.get(), &event, &next));
-  message_loop_.RunUntilIdle();
-  ASSERT_TRUE(event.IsSignaled());
-  EXPECT_EQ("/ic:id/name1", next.value());
-
-  event.Reset();
-
-  // This method will block until it is sure there are no more items.
-  task_runner_->PostTask(FROM_HERE,
-                         base::Bind(&EnumerateAndSignal,
-                         enumerator.get(), &event, &next));
-  message_loop_.RunUntilIdle();
-  ASSERT_FALSE(event.IsSignaled());
+TEST_F(MTPDeviceDelegateImplMacTest, TestDownload) {
+  ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+  base::Time t1 = base::Time::Now();
+  base::PlatformFileInfo info;
+  info.size = 4;
+  info.is_directory = false;
+  info.is_symbolic_link = false;
+  info.last_modified = t1;
+  info.last_accessed = t1;
+  info.creation_time = t1;
+  std::string kTestFileName("filename");
+  scoped_nsobject<MockMTPICCameraFile> picture1(
+      [[MockMTPICCameraFile alloc]
+          init:base::SysUTF8ToNSString(kTestFileName)]);
+  [camera_ addMediaFile:picture1];
+  delegate_->ItemAdded(kTestFileName, info);
   delegate_->NoMoreItems();
-  event.Wait();
-  ASSERT_TRUE(event.IsSignaled());
-  EXPECT_TRUE(next.empty());
-  message_loop_.RunUntilIdle();
+
+  EXPECT_EQ(base::PLATFORM_FILE_OK, ReadDir(base::FilePath(kDevicePath)));
+  ASSERT_EQ(1U, file_list_.size());
+  ASSERT_EQ("/ic:id/filename", file_list_[0].name);
+
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND,
+            DownloadFile(base::FilePath("/ic:id/nonexist"),
+                         temp_dir_.path().Append("target")));
+
+  EXPECT_EQ(base::PLATFORM_FILE_OK,
+             DownloadFile(base::FilePath("/ic:id/filename"),
+                          temp_dir_.path().Append("target")));
+  std::string contents;
+  EXPECT_TRUE(file_util::ReadFileToString(temp_dir_.path().Append("target"),
+                                          &contents));
+  EXPECT_EQ(kTestFileContents, contents);
 }

webkit/fileapi/media/mtp_device_file_system_config.h

@@ -17,7 +17,7 @@
 // Use asynchronous MTP device delegate API.
 // TODO(kmadhusu): remove this define and make this default.
 // Note that OS_LINUX implies OS_CHROMEOS
-#if defined(OS_WIN) || defined(OS_LINUX)
+#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
 #define USE_MTP_DEVICE_ASYNC_DELEGATE
 #endif