[Media Galleries] Filesystem interface for Mac PTP/MTP devices using ImageCaptureCore (part 3)

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/system_monitor/image_capture_device.h"
+#include "chrome/browser/system_monitor/image_capture_device_manager.h"
+#include "chrome/browser/system_monitor/media_storage_util.h"
+#include "content/public/browser/browser_thread.h"
+
+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 CameraDeviceInterface

[sail, 2012/12/26 21:30:43]

The "Interface" part of the name doesn't add anything. Maybe
"ImageCaptureDeviceListenerImpl" instead?

[Greg Billock, 2013/01/03 22:41:32]

Done, but just to "DeviceListenerImpl" to economize on space a bit. OK?

[sail, 2013/01/03 23:10:43]

Ok.

+    : public ImageCaptureDeviceListener,
+      public base::SupportsWeakPtr<CameraDeviceInterface> {

[sail, 2012/12/26 21:30:43]

Should avoid multiple inheritance. Use base::WeakPtrFactory instead?

[Greg Billock, 2013/01/03 22:41:32]

Could; this is pretty common though, and easy enough. I think it's basically
intended to be used this way.

[sail, 2013/01/03 23:10:43]

That's not a good argument. Please avoid using multiple inheritance when ever
possible.

Also, in this case you don't need to expose the weak pointer to other classes.
Using composition here better enforces that.

[Greg Billock, 2013/01/04 18:05:05]

Neither of these are good arguments. The listener "superclass" is an interface,
which are fine to multiply-inherit. If you argue that SupportsWeakPtr<T>
shouldn't even exist in the first place, that's a bit more compelling, although
the usage here is just about exactly the counterargument about why SWP
exists--the listener expects to be used as a weak pointer.

As to exposing the class as a weak pointer, I see what you're saying, I just
don't think it's accurate -- anyone with a pointer can trivially make a weak
pointer. There's nothing beyond ease-of-use and a mild documentation reason to
have SupportsWeakPtr, so like I said, If you're an enemy of the whole idea, I
can maybe even get behind that. :-) I just don't buy multiple inheritance or
obstacles-to-clients as valid arguments.

+ public:
+  CameraDeviceInterface(MTPDeviceDelegateImplMac* delegate,
+                        base::SequencedTaskRunner* task_runner)
+      : delegate_(delegate),
+        task_runner_(task_runner) {}
+  virtual ~CameraDeviceInterface() {}
+
+  void OpenCameraSession(const std::string& device_id);
+  void CloseCameraSessionAndDelete();
+
+  void DownloadFile(const std::string& name, const 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;
+
+ private:
+  scoped_nsobject<ImageCaptureDevice> camera_device_;
+
+  // Weak pointer
+  MTPDeviceDelegateImplMac* delegate_;
+
+  // Weak pointer
+  base::SequencedTaskRunner* task_runner_;
+};
+
+void CameraDeviceInterface::OpenCameraSession(const std::string& device_id) {
+  camera_device_.reset(
+      [ImageCaptureDeviceManager::deviceForUUID(device_id) retain]);
+  [camera_device_ setListener:AsWeakPtr()];
+  [camera_device_ open];
+}
+
+void CameraDeviceInterface::CloseCameraSessionAndDelete() {
+  if (camera_device_.get()) {

[sail, 2012/12/26 21:30:43]

don't need this, method calls to nil are no-op

[Greg Billock, 2013/01/03 22:41:32]

Done.

+    [camera_device_ close];
+    [camera_device_ setListener:base::WeakPtr<CameraDeviceInterface>()];
+  }
+
+  delete this;
+}
+
+void CameraDeviceInterface::DownloadFile(const std::string& name,
+                                         const FilePath& local_path) {
+  [camera_device_ downloadFile:name localPath:local_path];
+}
+
+void CameraDeviceInterface::ItemAdded(const std::string& name,
+                                      const base::PlatformFileInfo& info) {
+  task_runner_->PostTask(FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::ItemAdded,
+                 base::Unretained(delegate_), name, info));
+}
+
+void CameraDeviceInterface::NoMoreItems() {
+  task_runner_->PostTask(FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::NoMoreItems,
+                 base::Unretained(delegate_)));
+}
+
+void CameraDeviceInterface::DownloadedFile(const std::string& name,
+                                           base::PlatformFileError error) {
+  task_runner_->PostTask(FROM_HERE,
+      base::Bind(&MTPDeviceDelegateImplMac::DownloadedFile,
+                 base::Unretained(delegate_), name, error));
+}
+
+void CameraDeviceInterface::DeviceRemoved() {
+  [camera_device_ close];
+  camera_device_.reset();
+}
+
+MTPDeviceDelegateImplMac::MTPDeviceDelegateImplMac(
+    const FilePath::StringType& location,
+    base::SequencedTaskRunner* media_task_runner)
+    : root_path_(location),
+      media_task_runner_(media_task_runner),
+      received_all_files_(false) {

[sail, 2012/12/26 21:30:43]

need to initialize enumerator_

[Greg Billock, 2013/01/03 22:41:32]

Done.

+  std::string device_id = FilePath(location).BaseName().value();

[sail, 2012/12/26 21:30:43]

instead of implicitly encoding the device ID in the file path it should just be
passed as an explicit parameter

[Greg Billock, 2013/01/03 22:41:32]

Yes. The machinery surrounding this doesn't allow that, but should. This is an
ugly adaptation.

[sail, 2013/01/03 23:10:43]

In that case can you move this out to the code that instantiates this class and
add a TODO there? There's no reason the new code needs to have all this.

This will also simplify testing.

[Greg Billock, 2013/01/04 18:05:05]

The outside code is in the File API, and introducing this dependency there is a
worse outrage, in my view. The whole situation is a serious encapsulation flaw
I'm attempting to fix in other ways, so I'm definitely not saying you're wrong
to object to it.

[sail, 2013/01/08 20:16:41]

Either way, it should be fixed before this can land.

[Greg Billock, 2013/01/16 01:15:29]

Moved to the factory method. I think that's cosmetically better, but a full
solution is out of the scope of this change. It's not appropriate to make this
change dependent on that.

On 2013/01/08 20:16:41, sail wrote:

+  MediaStorageUtil::Type type;
+  bool cracked = MediaStorageUtil::CrackDeviceId(device_id, &type, &device_id_);
+  DCHECK(cracked);
+  DCHECK_EQ(MediaStorageUtil::MAC_IMAGE_CAPTURE, type);
+
+  // 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 CameraDeviceInterface(this, media_task_runner));
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&CameraDeviceInterface::OpenCameraSession,
+                 base::Unretained(camera_interface_.get()),
+                 device_id_));
+}
+
+MTPDeviceDelegateImplMac::~MTPDeviceDelegateImplMac() {
+  DCHECK(media_task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(enumerator_ == NULL);
+}
+
+base::PlatformFileError MTPDeviceDelegateImplMac::GetFileInfo(
+    const FilePath& file_path,
+    base::PlatformFileInfo* file_info) {
+  base::hash_map<FilePath::StringType,
+                 base::PlatformFileInfo>::const_iterator i =
+      file_info_.find(file_path.value());
+  if (i != file_info_.end()) {
+    file_info->size = i->second.size;

[sail, 2012/12/26 21:30:43]

would this work:
  *file_info = i->second

[Greg Billock, 2013/01/03 22:41:32]

I think so. I thought I did that, but maybe I merged badly or it was elsewhere.

+    file_info->is_directory = i->second.is_directory;
+    file_info->is_symbolic_link = i->second.is_symbolic_link;
+    file_info->last_modified = i->second.last_modified;
+    file_info->last_accessed = i->second.last_accessed;
+    file_info->creation_time = i->second.creation_time;
+    return base::PLATFORM_FILE_OK;
+  } else {
+    return base::PLATFORM_FILE_ERROR_NOT_FOUND;

[sail, 2012/12/26 21:30:43]

early return above instead?

[Greg Billock, 2013/01/03 22:41:32]

Done.

+  }
+}
+
+scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
+MTPDeviceDelegateImplMac::CreateFileEnumerator(const FilePath& root,
+                                               bool recursive) {
+  DCHECK(!enumerator_);
+  enumerator_ = new Enumerator(this);
+  return make_scoped_ptr(enumerator_)
+      .PassAs<fileapi::FileSystemFileUtil::AbstractFileEnumerator>();
+}
+
+base::PlatformFileError MTPDeviceDelegateImplMac::CreateSnapshotFile(
+    const FilePath& device_file_path,
+    const FilePath& local_path,
+    base::PlatformFileInfo* file_info) {
+  std::string name = device_file_path.BaseName().value();

[sail, 2012/12/26 21:30:43]

should move to first use

[Greg Billock, 2013/01/03 22:41:32]

eliminated. I think I had earlier code that used it, but anyway...

On 2012/12/26 21:30:43, sail wrote:

+  base::PlatformFileError error = GetFileInfo(device_file_path, file_info);
+  if (error != base::PLATFORM_FILE_OK)
+    return error;
+
+  // Set up to wait for download.
+  base::WaitableEvent waiter(true, false);
+  download_events_[name] = &waiter;
+  // Start the download in the UI thread.
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&CameraDeviceInterface::DownloadFile,
+                 base::Unretained(camera_interface_.get()),
+                 name, local_path));
+  waiter.Wait();
+  download_events_.erase(download_events_.find(name));
+  base::hash_map<std::string, base::PlatformFileError>::iterator iter =
+      download_errors_.find(name);
+  error = iter->second;
+  download_errors_.erase(iter);
+
+  // Modify the last modified time to null. This prevents the time stamp
+  // verification in LocalFileStreamReader.
+  file_info->last_modified = base::Time();
+
+  return error;
+}
+
+void MTPDeviceDelegateImplMac::CancelPendingTasksAndDeleteDelegate() {
+  // Artificially pretend that we have already gotten all items we're going
+  // to get.
+  NoMoreItems();
+
+  // Artificially wake up any downloads pending with an error code.
+  for (base::hash_map<std::string, base::WaitableEvent*>::const_iterator iter =
+           download_events_.begin(); iter != download_events_.end(); ++iter) {
+    download_errors_[iter->first] = base::PLATFORM_FILE_ERROR_FAILED;
+    iter->second->Signal();
+  }
+
+  // Schedule the camera session to be closed and the interface deleted.
+  content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE,
+      base::Bind(&CameraDeviceInterface::CloseCameraSessionAndDelete,
+                 base::Unretained(camera_interface_.release())));
+
+  media_task_runner_->DeleteSoon(FROM_HERE, this);
+}
+
+void MTPDeviceDelegateImplMac::ItemAdded(
+    const std::string& name, const base::PlatformFileInfo& info) {
+  // Make sure if we're canceled and enumerators are awake, that
+  // they 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;
+
+  if (info.is_directory)
+    return;
+  FilePath fp = root_path_.Append(name);

[sail, 2012/12/26 21:30:43]

avoid abbreviations (more below)

[Greg Billock, 2013/01/03 22:41:32]

These are just aliasing abbreviations, like "i" or "iter". I think abbreviating
makes that more clear. 

[sail, 2013/01/03 23:10:43]

i and iter are used in all C++ programs. FilePath is not, it's a custom class.

[Greg Billock, 2013/01/04 18:05:05]

OK, I can make them more self-documenting names, anyway, which will be good.

+  file_info_[fp.value()] = info;
+  file_paths_.push_back(fp.value());
+
+  if (enumerator_)
+    enumerator_->ItemsChanged();
+}
+
+void MTPDeviceDelegateImplMac::NoMoreItems() {
+  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;
+
+  download_errors_[name] = error;
+  download_events_[name]->Signal();
+}
+
+FilePath MTPDeviceDelegateImplMac::GetFile(size_t index) {
+  if (index >= file_paths_.size())
+    return FilePath();
+  else
+    return FilePath(file_paths_[index]);
+}
+
+bool MTPDeviceDelegateImplMac::HasAllFiles() {
+  return received_all_files_;
+}
+
+void MTPDeviceDelegateImplMac::RemoveEnumerator(Enumerator* enumerator) {
+  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);
+}
+
+FilePath MTPDeviceDelegateImplMac::Enumerator::Next() {
+  FilePath fp = delegate_->GetFile(position_);
+  while (fp.empty() && !delegate_->HasAllFiles()) {
+    wait_for_items_.Wait();
+    fp = delegate_->GetFile(position_);
+  }
+
+  position_++;
+  return fp;
+}
+
+int64 MTPDeviceDelegateImplMac::Enumerator::Size() {
+  FilePath fp = delegate_->GetFile(position_ - 1);
+  base::PlatformFileInfo info;
+  delegate_->GetFileInfo(fp, &info);
+  return info.size;
+  return 0;

[sail, 2012/12/26 21:30:43]

remove

[Greg Billock, 2013/01/03 22:41:32]

Oops! What happened to dead code detection...

On 2012/12/26 21:30:43, sail wrote:

[sail, 2013/01/03 23:10:43]

I'm not sure how the dead code detection works. The only thing I've seen cause
an error is having unreferenced functions and variables.

[Greg Billock, 2013/01/04 18:05:05]

Two returns means the second must be unreachable; I know I've seen compiler
warnings of things like that...

+}
+
+base::Time MTPDeviceDelegateImplMac::Enumerator::LastModifiedTime() {
+  FilePath fp = delegate_->GetFile(position_ - 1);
+  base::PlatformFileInfo info;
+  delegate_->GetFileInfo(fp, &info);
+  return info.last_modified;
+}
+
+bool MTPDeviceDelegateImplMac::Enumerator::IsDirectory() {
+  FilePath fp = delegate_->GetFile(position_ - 1);
+  base::PlatformFileInfo info;
+  delegate_->GetFileInfo(fp, &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) {
+  cb.Run(new MTPDeviceDelegateImplMac(device_location, media_task_runner));
+}
+
+}  // namespace chrome
+