[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. ScopedMTPDeviceMapEntry class
+// manages the lifetime of this object via MTPDeviceMapService class.

[Peter Kasting, 2012/12/03 20:13:00]

Nit: I'm not sure I understand this sentence... maybe the last three words
should be removed, or the two classnames be swapped?

[kmadhusu, 2012/12/15 02:34:56]

This sentence is an implementation detail. This sentence is not required in the
file header comment. Removed the sentence from here. 

+// This object is constructed on UI thread. All the device operations are

[Peter Kasting, 2012/12/03 20:13:00]

Nit: on UI -> on the UI

[kmadhusu, 2012/12/15 02:34:56]

This sentence is no longer used.

+// performed on the blocking thread. A portable device can have multiple
+// data partitions. A user can register each partition as a media file system.
+// This class is instantitated per MTP device storage.

[Peter Kasting, 2012/12/03 20:13:00]

Nit: instantitated -> instantiated

What does "per MTP device storage" mean?  You haven't used the word "storage"
before in this comment so it doesn't mean much to me.

[kmadhusu, 2012/12/15 02:34:56]

Fixed. 
instantitated -> instantiated
storage -> 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>
+
+#include "base/platform_file.h"
+#include "base/string16.h"
+#include "base/synchronization/lock.h"
+#include "base/win/scoped_comptr.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 MTP storage to complete isolated file system
+// operations. This class contains platform specific code to communicate with
+// the attached MTP storage. This class supports weak pointers, because the
+// base class supports weak pointers.
+class MTPDeviceDelegateImplWin : public fileapi::MTPDeviceDelegate {
+ public:
+  // Constructed on UI thread. Defer the device initializations until the first
+  // file operation request. Do all the initializations in LazyInit() function.
+  explicit MTPDeviceDelegateImplWin(const string16& pnp_device_id);
+
+  // 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;
+  virtual void CancelPendingTasksAndDeleteDelegate() OVERRIDE;
+  virtual base::WeakPtr<fileapi::MTPDeviceDelegate> GetAsWeakPtrOnIOThread()

[Peter Kasting, 2012/12/03 20:13:00]

You're vending a weak pointer on a different thread (IO) than the object is
created on (UI) or destroyed on (task runner).  Don't do this (see comments in
.cc file for more detail).  Weak pointers should only be created and
dereferenced on one thread, and it should be the thread the object is
created/destroyed on.  Also see comment on DeleteDelegateOnTaskRunner().

[kmadhusu, 2012/12/15 02:34:56]

This class no longer supports weak pointers. This class mostly lives on media
task runner thread except  for CancelPendingTasksAndDeleteDelegate() which
happens on the UI thread.

+      OVERRIDE;
+
+ private:
+  // Destructed via DeleteDelegateOnTaskRunner(). Do all the clean up in
+  // DeleteDelegateOnTaskRunner().
+  virtual ~MTPDeviceDelegateImplWin();
+
+  // Opens the device for communication. This function is called on the
+  // sequenced task runner. Returns true if the device is ready for
+  // communication, else false.

[Peter Kasting, 2012/12/03 20:13:00]

Nit: "Returns whether the device is ready for communication." or "Returns
whether initialization succeeded."

[kmadhusu, 2012/12/15 02:34:56]

Done.

+  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 object id of "DCIM" folder.

[Peter Kasting, 2012/12/03 20:13:00]

Nit: of -> of the

Is the object ID "DCIM"?  Or is it something else?

[kmadhusu, 2012/12/15 02:34:56]

DCIM is the folder object name. Based on the stated example, this function
returns the object id of the "DCIM" folder object.

+  //
+  // Returns an empty string, if the device is detached while the request is in

[Peter Kasting, 2012/12/03 20:13:00]

Nit: Remove comma

[kmadhusu, 2012/12/15 02:34:56]

Done.

+  // progress.
+  string16 GetFileObjectIdFromPath(const string16& file_path);
+
+  // Deletes the delegate on the task runner thread. Called by

[Peter Kasting, 2012/12/03 20:13:00]

!! Objects should be created and destroyed on the same thread, especially when
they support weak pointers.  So if you create this on the UI thread it should be
destroyed on the UI thread.

Here's the way you might implement this.  Make object creation happen on the UI
thread, declare all public APIs to be callable on the UI thread, and have a weak
pointer factory that vends weak pointers on the UI thread.  (We recommend using
weak pointer factories instead of SupportsWeakPtr.)

Have the object that creates this grab a weak pointer and pass it to the object
that needs to call on the IO thread.  That object can't deref the weak pointer,
but it can do something like:

  ui_loop->PostTaskAndReplyWithResult(
      FROM_HERE,
      base::Bind(&MTPDeviceDelegateImplWin::Func, delegate_weak_ptr, args),
      base::Bind(&MyClass::MyFunc, GetWeakPtr(), args));

This will call MTPDeviceDelegateImplWin::Func() on the UI thread if the delegate
still exists, then call back to MyClass::MyFunc() on the IO thread if the
calling object still exists.

Then internally, when you need to implement something as a call to the blocking
pool, you do something similar:

  media_task_runner_->PostTaskAndReplyWithResult(
      FROM_HERE,
      base::Bind(&MTPDeviceDelegateImplWin::StaticFunc, args),
      base::Bind(&MTPDeviceDelegateImplWin::ReplyFunc, GetWeakPtr(), args));

StaticFunc() should be static because it's not safe to touch any members unless
we guarantee that the object will outlive any blocking-pool-posted tasks, which
we don't.  Instead you pass in and receive data as arguments/return values.

You can change PostTaskAndReplyWithResult() to PostTaskAndReply() or just
PostTask() and optionally pass in closures to be called back at completion time
if you need to chain these sorts of things together (e.g. implement a call from
the IO thread object as a call-and-reply to the blocking pool).

In this world, you can still have something like LazyInit(), but you might do
something like this (exact syntax may not be right):

  std::stack<std::pair<Closure, Closure>> pending_tasks_;

  void MyFuncThatNeedsLazyInit() {
    EnsureInitAndThenRunTask(base::Bind(...), base::Bind(...));
  }
  void EnsureInitAndThenRunTask(const Closure& call_closure,
                                const Closure& reply_closure) {
    if (initialized_) {
      media_task_runner_->PostTaskAndReply(
          FROM_HERE, call_closure, reply_closure);
    }
    pending_tasks_.push(make_pair(call_closure, reply_closure));
    if (!pending_init_) {
      pending_init_ = true;
      media_task_runner_->PostTaskAndReplyWithResult(
          FROM_HERE, &LazyInit,
          base::Bind(&OnInitCompleted, GetWeakPtr()));
    }
  }
  void OnInitCompleted(bool succeeded) {
    pending_init_ = false;
    initialized_ = succeeded;
    if (!initialized_) {
      pending_tasks_.clear();
    } else {
      // run all pending tasks
    }
  }

This allows you to (a) ensure that only one call to LazyInit is active at any
time, (b) make LazyInit static, (c) keep the threading/memory usage safe, and
(d) still call back to all desired tasks once init succeeds.

[kmadhusu, 2012/12/15 02:34:56]

Removed DeleteDelegateOnTaskRunner() function. 

+  // CancelPendingTasksAndDeleteDelegate(). Performs clean up that needs to
+  // happen on the task runner thread.
+  void DeleteDelegateOnTaskRunner();

[Peter Kasting, 2012/12/03 20:13:00]

Nit: I suggest adding "Thread" to the end of this function name

[kmadhusu, 2012/12/15 02:34:56]

This function is no longer used.

+
+  // 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 as a file system,
+  // e.g. "\\MTP:StorageSerial:SID-{10001,E,9823}:237483".
+  const string16 registered_device_path_;
+
+  // The MTP device storage 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_;
+
+  // The portable device.
+  base::win::ScopedComPtr<IPortableDevice> device_;
+
+  // The lock to protect |cancel_pending_tasks_|. |cancel_pendings_tasks_| is
+  // set on IO 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_;

[Peter Kasting, 2012/12/03 20:13:00]

This seems like a job for a CancellationFlag.

[kmadhusu, 2012/12/15 02:34:56]

CancellationFlag can only be set on the thread it is instantiated. In our case,
CancellationFlag is created on sequenced task runner thread and has to be set on
UI thread. Therefore, I couldn't use it as such.

+
+  DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplWin);
+};
+
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_MEDIA_GALLERY_WIN_MTP_DEVICE_DELEGATE_IMPL_WIN_H_