[Media Gallery] Added code to support mtp device media file system on Windows.

chrome/browser/media_gallery/win/mtp_device_delegate_impl_win.h

+// 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.
+//
+// This class supports media transfer protocol (MTP) device file system
+// operations. This class communicates with the portable device and completes
+// the requested file system operation. A portable device can have multiple
+// data partitions. A user can register each partition as a media file system.
+// This class is instantiated per MTP device partition.
+
+#ifndef CHROME_BROWSER_MEDIA_GALLERY_WIN_MTP_DEVICE_DELEGATE_IMPL_WIN_H_
+#define CHROME_BROWSER_MEDIA_GALLERY_WIN_MTP_DEVICE_DELEGATE_IMPL_WIN_H_
+
+#include <portabledeviceapi.h>

[Peter Kasting, 2012/12/19 20:17:58]

Nit: In general can we avoid such #includes by forward-declaring things like
IPortableDevice?  (Several files)

[kmadhusu, 2012/12/20 20:37:29]

How to forward declare this interface? There is no "portabledeviceidl.h" file. I
tried to do the forward declaration using "interface IPortableDevice". But I get
the following compile error:

d:\kmadhusu\home\kmadhusu\chromium\src\base\win\scoped_comptr.h(19) : error
C2787: 'IPortableDevice' : no GUID has been associated with this object
d:\kmadhusu\home\kmadhusu\chromium\src\chrome\browser\media_gallery\win\mtp_device_delegate_impl_win.cc(183)
: error C2664: 'chrome::MTPDeviceOperatio
nsUtil::OpenDevice' : cannot convert parameter 2 from
'base::win::ScopedComPtr<Interface> *' to 'base::win::ScopedComPtr<Interface> *'
        with
        [
            Interface=IPortableDevice
        ]
        Types pointed to are unrelated; conversion requires reinterpret_cast,
C-style cast or function-style cast
ninja: build stopped: subcommand failed.

[Peter Kasting, 2012/12/20 21:24:20]

I was thinking we could just use "class IPortableDevice".  If the compiler
doesn't like that, just ignore this comment.

[kmadhusu, 2012/12/21 01:45:23]

Compiler is not happy when I specify "class IPortableDevice". Ignoring this
review comment.

+#include <string>
+
+#include "base/platform_file.h"
+#include "base/sequenced_task_runner_helpers.h"
+#include "base/string16.h"
+#include "base/synchronization/lock.h"
+#include "base/win/scoped_comptr.h"
+#include "chrome/browser/media_gallery/mtp_device_delegate_impl.h"
+#include "webkit/fileapi/file_system_file_util.h"
+#include "webkit/fileapi/media/mtp_device_delegate.h"
+
+class FilePath;
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+namespace chrome {
+
+// This class communicates with the MTP device to complete isolated file system
+// operations. This class contains platform specific code to communicate with
+// the attached MTP device. Instantiate this class per MTP device storage
+// partition using CreateMTPDeviceDelegate(). This object lives on a sequenced
+// task runner thread, except for CancelPendingTasksAndDeleteDelegate() which
+// happens on the UI thread.
+class MTPDeviceDelegateImplWin : public fileapi::MTPDeviceDelegate {
+ private:
+  friend void CreateMTPDeviceDelegate(const string16&,
+                                      base::SequencedTaskRunner*,
+                                      const CreateMTPDeviceDelegateCallback&);
+  friend class base::DeleteHelper<MTPDeviceDelegateImplWin>;
+
+  // Defer the device initializations until the first file operation request.
+  // Do all the initializations in LazyInit() function.
+  MTPDeviceDelegateImplWin(const string16& pnp_device_id,
+                           base::SequencedTaskRunner* media_task_runner);
+
+  // Destructed via CancelPendingTasksAndDeleteDelegate().
+  virtual ~MTPDeviceDelegateImplWin();
+
+  // MTPDeviceDelegate:
+  virtual base::PlatformFileError GetFileInfo(
+      const FilePath& file_path,
+      base::PlatformFileInfo* file_info) OVERRIDE;
+  virtual scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
+      CreateFileEnumerator(const FilePath& root,
+                           bool recursive) OVERRIDE;
+  virtual base::PlatformFileError CreateSnapshotFile(
+      const FilePath& device_file_path,
+      const FilePath& local_path,
+      base::PlatformFileInfo* file_info) OVERRIDE;
+  virtual base::SequencedTaskRunner* GetMediaTaskRunner() OVERRIDE;

[Peter Kasting, 2012/12/19 20:17:58]

It looks like in codesearch this isn't called anywhere, and it's not called in
this patch either.  Can we nuke this function entirely (including from the
parent class)?

[kmadhusu, 2012/12/20 20:37:29]

Thanks for pointing this out. Removed this function from the base class and from
here. 

+  virtual void CancelPendingTasksAndDeleteDelegate() OVERRIDE;
+
+  // Returns whether the device is ready for communication.
+  bool LazyInit();
+
+  // Returns the object id of the file object specified by the |file_path|,
+  // e.g. if the |file_path| is "\\MTP:StorageSerial:SID-{1001,,192}:125\DCIM"
+  // and |registered_device_path_| is "\\MTP:StorageSerial:SID-{1001,,192}:125",
+  // this function returns the identifier of the "DCIM" folder object.
+  //
+  // Returns an empty string if the device is detached while the request is in
+  // progress.
+  string16 GetFileObjectIdFromPath(const string16& file_path);
+
+  // The Pnp Device Id, used to open the device for communication,
+  // e.g. "\\?\usb#vid_04a9&pid_3073#12#{6ac27878-a6fa-4155-ba85-f1d4f33}".
+  string16 pnp_device_id_;
+
+  // The media file system root path, which is obtained during the registration
+  // of MTP device storage partition as a file system,
+  // e.g. "\\MTP:StorageSerial:SID-{10001,E,9823}:237483".
+  const string16 registered_device_path_;
+
+  // The MTP device storage partition object identifier, used to enumerate the
+  // storage contents, e.g. "s10001".
+  string16 storage_object_id_;
+
+  // The task runner used to execute tasks that may take a long time and thus
+  // should not be performed on the UI thread.
+  scoped_refptr<base::SequencedTaskRunner> media_task_runner_;

[Peter Kasting, 2012/12/19 20:17:58]

How come we actually need this anyway?  It seems like
CancelPendingTasksAndDeleteDelegate() could just use GetBlockingPool() directly,
as could any other places that reference this.

[kmadhusu, 2012/12/20 20:37:29]

This comment is misleading. This reference pointer is mainly used to
(1) make sure the file system operations are performed on the blocking pool
thread.
(2) destruct the MTPDeviceDelegateImplWin object on the blocking pool thread.

If I don't have a reference to task_runner here, I need to do the following:

bool IsMediaTaskRunnerThread() {
  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
  base::SequencedWorkerPool::SequenceToken media_sequence_token =
      pool->GetNamedSequenceToken(fileapi::kMediaTaskRunnerName);
  return pool->IsRunningSequenceOnCurrentThread(media_sequence_token);
}

scoped_refptr<base::SequencedTaskRunner> GetSequencedTaskRunner() {
  base::SequencedWorkerPool* pool = content::BrowserThread::GetBlockingPool();
  base::SequencedWorkerPool::SequenceToken media_sequence_token =
      pool->GetNamedSequenceToken(fileapi::kMediaTaskRunnerName);
  return pool->GetSequencedTaskRunner(media_sequence_token);
}

void MTPDeviceDelegateImplWin::CancelPendingTasksAndDeleteDelegate() {
  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
  {
    base::AutoLock locked(cancel_tasks_lock_);
    cancel_pending_tasks_ = true;
  }
  GetSequencedTaskRunner()->DeleteSoon(FROM_HERE, this);
}

bool MTPDeviceDelegateImplWin::LazyInit() {
  DCHECK(IsMediaTaskRunnerThread());
  ...
}

We are already creating the sequenced task runner to construct the
MTPDeviceDelegateImplWin object. So, it's better to have a reference here and
use it when required.

[Peter Kasting, 2012/12/20 21:24:20]

I talked to rsleevi a bunch about this so I could fully understand all the
tradeoffs in different solutions.  I'm fine with what you're doing here.  Maybe
this comment should simply be something like:

"The task runner where most of the class lives and runs (save for
CancelPendingTasksAndDeleteDelegate())."

[kmadhusu, 2012/12/21 01:45:23]

Fixed the comment.

+
+  // The portable device.
+  base::win::ScopedComPtr<IPortableDevice> device_;
+
+  // The lock to protect |cancel_pending_tasks_|. |cancel_pendings_tasks_| is

[Peter Kasting, 2012/12/19 20:17:58]

Nit: Extra 's'

[kmadhusu, 2012/12/20 20:37:29]

Fixed.

+  // set on the UI thread when the
+  // (1) Browser application is in shutdown mode (or)
+  // (2) Last extension using this MTP device is destroyed (or)
+  // (3) Attached MTP device is removed (or)
+  // (4) User revoked the MTP device gallery permission.
+  base::Lock cancel_tasks_lock_;
+  bool cancel_pending_tasks_;
+
+  DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplWin);
+};
+
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_MEDIA_GALLERY_WIN_MTP_DEVICE_DELEGATE_IMPL_WIN_H_