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

chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.mm

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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 {
 
 // 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
 // interactions with it are done on the UI thread, but it may be
 // created/destroyed on another thread.
 class MTPDeviceDelegateImplMac::DeviceListener
     : public ImageCaptureDeviceListener,
       public base::SupportsWeakPtr<DeviceListener> {
  public:
   DeviceListener(MTPDeviceDelegateImplMac* delegate)
       : delegate_(delegate) {}
   virtual ~DeviceListener() {}
 
   void OpenCameraSession(const std::string& device_id);
   void CloseCameraSessionAndDelete();
 
   void DownloadFile(const std::string& name, const base::FilePath& local_path);
 
   // ImageCaptureDeviceListener
   virtual void ItemAdded(const std::string& name,
                          const base::PlatformFileInfo& info) OVERRIDE;
   virtual void NoMoreItems() OVERRIDE;
   virtual void DownloadedFile(const std::string& name,
                               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_;
 
   // Weak pointer
   MTPDeviceDelegateImplMac* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceListener);
 };
 
 void MTPDeviceDelegateImplMac::DeviceListener::OpenCameraSession(
@@ skipping 13 matching lines @@
 
 void MTPDeviceDelegateImplMac::DeviceListener::DownloadFile(
     const std::string& name,
     const base::FilePath& local_path) {
   [camera_device_ downloadFile:name localPath:local_path];
 }
 
 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.
   base::PlatformFileInfo info;
   info.is_directory = true;
   file_info_[root_path_.value()] = info;
 
   camera_interface_.reset(new DeviceListener(this));
   content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
       base::Bind(&DeviceListener::OpenCameraSession,
                  base::Unretained(camera_interface_.get()),
                  device_id_));
 }
 
 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;
+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;
 
-  // 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;
+  if (received_all_files_)
+    NotifyReadDir();
+
+  // 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(&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;
+
+  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;
   }
 
-  // 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_;
-  }
-
-  // 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() {
+  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;
 
   // TODO(gbillock): Currently we flatten all files into a single
   // directory. That's pretty much how PTP devices work, but if we want
   // to support ImageCapture for USB, we need to change this.
   if (info.is_directory)
     return;
 
   base::FilePath item_filename = root_path_.Append(name);
   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();
+  NotifyReadDir();
 }
 
+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::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));
+
+    read_directory_success_callback_.Reset();
+    read_directory_error_callback_.Reset();
+  }
+}
+
+// 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();
+  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();
+  }
 }
 
-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);
-}
-
-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_);
-  }
-
-  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;
-}
-
-void MTPDeviceDelegateImplMac::Enumerator::ItemsChanged() {
-  wait_for_items_.Signal();
-}
-
-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,
                                                  &type, &device_id);
   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