[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..a486a54daae76b35bef58258fd199bd290e6dba6 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,14 +5,22 @@
 #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"
+
+using fileapi::MTPDeviceAsyncDelegate;
+typedef MTPDeviceAsyncDelegate::CreateSnapshotFileSuccessCallback
+    CreateSnapshotFileSuccessCallback;
+typedef MTPDeviceAsyncDelegate::ErrorCallback ErrorCallback;
+typedef MTPDeviceAsyncDelegate::GetFileInfoSuccessCallback
+    GetFileInfoSuccessCallback;

[kmadhusu, 2013/02/26 17:59:15]

Can you declare these type definitions in an anonymous namespace?

[Greg Billock, 2013/02/26 22:02:09]

Done.

+typedef MTPDeviceAsyncDelegate::ReadDirectorySuccessCallback
+    ReadDirectorySuccessCallback;
 
 namespace chrome {
 
@@ -42,6 +50,8 @@ class MTPDeviceDelegateImplMac::DeviceListener
                               base::PlatformFileError error) OVERRIDE;
   virtual void DeviceRemoved() OVERRIDE;
 
+  virtual void ResetDelegate();

[kmadhusu, 2013/02/26 17:59:15]

OVERRIDE?

[Greg Billock, 2013/02/26 22:02:09]

No, this isn't an ImageCaptureDeviceListener method. It's around for destruction
behavior. I'll add a note about that.

+
  private:
   scoped_nsobject<ImageCaptureDevice> camera_device_;
 
@@ -75,34 +85,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) {
 

[kmadhusu, 2013/02/26 17:59:15]

Please add DCHECKs in all member functions to make sure they are called on the
right thread.

[Greg Billock, 2013/02/26 22:02:09]

The important ones are the state-modifying ones. I added them there. The others
are not needed.

   // Make a synthetic entry for the root of the filesystem.
@@ -118,99 +132,146 @@ MTPDeviceDelegateImplMac::MTPDeviceDelegateImplMac(
 }
 
 MTPDeviceDelegateImplMac::~MTPDeviceDelegateImplMac() {
-  DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
-  DCHECK(enumerator_ == NULL);
 }
 
-base::PlatformFileError MTPDeviceDelegateImplMac::GetFileInfo(
+void ForwardGetFileInfo(

[kmadhusu, 2013/02/26 17:59:15]

Can this go to the anonymous namespace?

[Greg Billock, 2013/02/26 22:02:09]

Done.

+    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);
+  }
+}
+
+// Called on the IO thread by the async MTP file util.
+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,
+      FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::GetFileInfoImpl,
+                 base::Unretained(this), file_path, info, error),
+      base::Bind(&ForwardGetFileInfo,

[kmadhusu, 2013/02/26 17:59:15]

Why this closure is not owning |error| and |info|? If you make this closure own
|error| and |info|, you can remove |info_deleter| and |error_deleter| from line
142 and 143 respectively.

[Greg Billock, 2013/02/26 22:02:09]

There aren't good copy constructors for PlatformFileInfo. Since I needed to pass
that one for deletion, I decided to do it for the other one too so it wouldn't
create a question.

+                 info, error, success_callback, error_callback));
+}
+
+void MTPDeviceDelegateImplMac::GetFileInfoImpl(
+    const base::FilePath& file_path,
+    base::PlatformFileInfo* file_info,
+    base::PlatformFileError* error) {
   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>();
+// Called on the IO thread by the async MTP file util.

[kmadhusu, 2013/02/26 17:59:15]

Please remove the comment and add a DCHECK in function definition.

[Greg Billock, 2013/02/26 22:02:09]

Added DCHECK to the UI thread functions.

+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),

[kmadhusu, 2013/02/26 17:59:15]

By using "base::Unretained(this)" in a bind call, you are promising that object
will be alive at the time of the call. 

From my understanding, MTPDeviceDelegateImplMac can be destructed before the
callback is called. We should use a weak pointer here. Please correct me if I am
wrong.

[Greg Billock, 2013/02/26 22:02:09]

Yes. I added a note in the header that the owner is basically guaranteeing
lifetime past response times. If that's not accurate, then yes, we need weak
pointers, but I think that's a fair thread-compatibility contract for the
filesystem API.

+                 root, success_callback, error_callback));
 }
 
-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::ReadDirectoryImpl(

[kmadhusu, 2013/02/26 17:59:15]

This class is currently accessed on multiple threads.

E.g: MTPDeviceDelegateImplMac::ReadDirectory is called on the IO thread and
MTPDeviceDelegateImplMac::ReadDirectoryImpl is called on the UI thread.

Can we make MTPDeviceDelegateImplMac as a single threaded class?

Sample:

namespace chrome {
namespace {

void ReadDirectoryOnUIThread(...) {
DCHECK(Called on UI thread);
}

}  // namespace

void MTPDeviceDelegateImplMac::ReadDirectory(...) {
  DCHECK(Called on IO thread);
  PostTaskOnUIThread(&ReadDirectoryOnUIThread, callback);
}

}  // namespace

[Greg Billock, 2013/02/26 22:02:09]

The IO-called methods don't do any state mutation -- they just forward directly
on the UI thread, so that keeps the class thread-compatible -- it can be created
by anyone, but thereafter only works on the UI thread.

+      const base::FilePath& root,
+      const ReadDirectorySuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  read_directory_success_callback_ = success_callback;
+  read_directory_error_callback_ = error_callback;
+
+  if (received_all_files_)
+    NotifyReadDir();
 
-  // Start the download in the UI thread.
+  // TODO(gbillock): Need to add a task on a timeout or something for
+  // the read dir failure?
+}
+
+// Called on the IO thread by the async MTP file util.
+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(&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::Bind(&MTPDeviceDelegateImplMac::DownloadFile,
+                 base::Unretained(this),
+                 device_file_path, local_path,
+                 success_callback, error_callback));
+}
+
+void MTPDeviceDelegateImplMac::DownloadFile(
+      const base::FilePath& device_file_path,
+      const base::FilePath& local_path,
+      const CreateSnapshotFileSuccessCallback& success_callback,
+      const ErrorCallback& error_callback) {
+  read_file_success_callback_ = success_callback;
+  read_file_error_callback_ = error_callback;
 
-  // Modify the last modified time to null. This prevents the time stamp
-  // verification in LocalFileStreamReader.
-  file_info->last_modified = base::Time();
+  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;
+  }
 
-  return error;
+  camera_interface_->DownloadFile(device_file_path.BaseName().value(),
+                                  local_path);
 }
 
 void MTPDeviceDelegateImplMac::CancelPendingTasksAndDeleteDelegate() {
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::CancelAndDelete,
+                 base::Unretained(this)));
+}
+
+void MTPDeviceDelegateImplMac::CancelAndDelete() {
   // 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;

[kmadhusu, 2013/02/26 17:59:15]

This class object is constructed on the IO thread and destructed on the UI
thread. We should avoid this. This class should completely live on the IO thread
and should do a call-and-reply to the UI thread to get the required device
details.

[Greg Billock, 2013/02/26 22:02:09]

I don't see that as a problem. The object is thread-compatible, meaning that it
guards itself from state corruption as long as a creator doesn't use it
uninitialized (which they shouldn't be able to do by using the offered
constructor). We could bounce creation to the UI thread, but I don't see that as
being a win. It'd keep it fully on one thread, but since it wants to handle
inbound IO-thread calls anyway, we'd either need a pair of objects that have
identical scopes and lifetimes, or use a dummy. I don't think either is clearer
than using forwarding continuations.

 }
 
+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 +285,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];
+  NotifyReadDir();
 }
 
-bool MTPDeviceDelegateImplMac::ReceivedAllFiles() {
-  base::AutoLock lock(mutex_);
-  return received_all_files_;
+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);
 }
 
-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);
-}
+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 +362,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