[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. MTPDeviceMapService owns this object.
+// This object is constructed and destructed on UI thread. All the device
+// operations are 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.
+
+#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/memory/ref_counted.h"
+#include "base/platform_file.h"
+#include "base/string16.h"
+#include "base/synchronization/cancellation_flag.h"
+#include "base/win/scoped_comptr.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.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 {
+
+// Helper class to communicate with MTP storage to complete isolated file system
+// operations. This class contains platform specific code to communicate with
+// the attached MTP storage. Instantiate this class per MTP storage.
+// This class is ref-counted, because MtpDeviceDelegate is ref-counted.

[Peter Kasting, 2012/11/01 21:38:37]

And does MtpDeviceDelegate really need to be ref-counted?

[kmadhusu, 2012/11/02 03:27:16]

Yes. This class is instantiated per MTP device storage. Extensions shares this
object to communicate with the device.

[Peter Kasting, 2012/11/02 04:08:11]

Please point me to somewhere that details the ownership model precisely.  The
fact that a class has multiple consumers does not mean it needs to be
refcounted.  There is almost always another way, which is why we ban refcounting
except with special permission.

[kmadhusu, 2012/11/29 23:58:14]

MTPDeviceDelegateImplWin is no longer ref-counted. Modified MTPDeviceDelegate to
support weak pointers. Therefore, MTPDeviceDelegateImplWin supports weak
pointers.

Ownership Model:
(1) MediaFileSystemRegistry manages ScopedMTPDeviceMapEntry object.
(2) ScopedMTPDeviceMapEntry manages the lifetime of MTPDeviceDelegateImpl object
via MTPDeviceMapService class.
(3) ScopedMTPDeviceMapEntry adds an entry in MTPDeviceMapService when the MTP
device is registered as a media gallery file system by an extension.
(4) ScopedMTPDeviceMapEntry removes an entry from MTPDeviceMapService, when the
browser is in shutdown mode or when the last extension using the device delegate
is destroyed or when the device is detached from the system or when the user
revoke the device gallery permissions.
(5) When an entry is removed from MTPDeviceMapService, the ownership of the
|MTPDeviceDelegateImpl| is handed off to the |MTPDeviceDelegateImpl| object
which takes care of deleting itself on the right thread.

+class MTPDeviceDelegateImplWin : public fileapi::MtpDeviceDelegate,
+                                 public content::NotificationObserver {
+ 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 fileapi::FileSystemFileUtil::AbstractFileEnumerator*

[Peter Kasting, 2012/11/01 21:38:37]

Nit: It would be nice for this function to return a scoped_ptr to indicate
that's it's passing ownership of a heap-allocated object to the caller.

[kmadhusu, 2012/11/02 03:27:16]

Will fix it in a separate CL. (This function signature matches
FileSystemFileUtil::CreateFileEnumerator function. If I am going to change this
function, its better to change all the instances of CreateFileEnumerator
function).

[kmadhusu, 2012/11/29 23:58:14]

Fixed.

+      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* media_task_runner() OVERRIDE;

[Peter Kasting, 2012/11/01 21:38:37]

Nit: virtual functions should never be named unix_hacker()-stlye (or inlined)

[kmadhusu, 2012/11/02 03:27:16]

Will fix in a separate CL. I have to change several places in webkit/fileapi
code where this function is used.

[kmadhusu, 2012/11/29 23:58:14]

Fixed.

+  virtual void DeleteOnCorrectThread() const OVERRIDE;
+
+ private:
+  friend struct fileapi::MtpDeviceDelegateDeleter;
+  friend class base::DeleteHelper<MTPDeviceDelegateImplWin>;
+
+  // Private because this class is ref-counted.
+  virtual ~MTPDeviceDelegateImplWin();
+
+  // content::NotificationObserver:
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // 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.
+  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"

[Peter Kasting, 2012/11/01 21:38:37]

Nit: If -> if

[kmadhusu, 2012/11/02 03:27:16]

Done.

+  // and |registered_device_path_| is "\\MTP:StorageSerial:SID-{1001,,192}:125",
+  // this function returns the object id of "DCIM" folder.
+  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 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_;
+
+  // Used to listen for NOTIFICATION_APP_TERMINATING message.

[Peter Kasting, 2012/11/01 21:38:37]

As in your previous change, you should not listen for APP_TERMINATING.  Instead
the class should simply be deleted at that point.  This will auto-cancel the
tasks.

[kmadhusu, 2012/11/02 03:27:16]

As I said earlier, this is a shared object. I cannot delete it directly. 

[Peter Kasting, 2012/11/02 04:08:11]

Again, I'm not convinced, especially after we moved away from APP_TERMINATING in
your other change.

Whoever owns this object should delete it during shutdown.  There should only be
one owner.

Even if for some reason you _had_ to refcount this, it should still simply
auto-concel on deletion, which would happen when the last ref was dropped.

Much like with refcounting itself, listening for app termination is pretty much
always wrong.

[kmadhusu, 2012/11/29 23:58:14]

Fixed. Code no longer listens to app termination message. 

+  content::NotificationRegistrar registrar_;
+
+  // Used to notify the blocking tasks about the application termination
+  // message.
+  base::CancellationFlag app_terminating_flag_;
+
+  DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplWin);
+};
+
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_MEDIA_GALLERY_WIN_MTP_DEVICE_DELEGATE_IMPL_WIN_H_