Redesigned and refactored ScopedMTPDeviceMapEntry, MTPDeviceMapService & MTPDeviceDelegate classes.

chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h

Index: chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h
diff --git a/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h b/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h
index 5f2802211642f6c4b3351c5804972e7e5bdf7064..cffb826c15d377eb213a8d2e2725c90bb2344f7c 100644
--- a/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h
+++ b/chrome/browser/media_gallery/mtp_device_delegate_impl_linux.h
@@ -7,10 +7,9 @@
 
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "base/platform_file.h"
 #include "base/synchronization/waitable_event.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"
 
@@ -22,19 +21,23 @@ 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.
-class MTPDeviceDelegateImplLinux : public fileapi::MTPDeviceDelegate,
-                                   public content::NotificationObserver {
+// This class communicates with the MTP storage to complete the isolated file
+// system operations. This class contains platform specific code to communicate
+// with the attached MTP storage. Instantiate this class per MTP storage. This
+// object is constructed on the UI thread. This object is operated and
+// destructed on the sequenced task runner thread. ScopedMTPDeviceMapEntry class
+// manages the lifetime of this object via MTPDeviceMapService class. This class
+// supports weak pointers because fileapi::MTPDeviceDelegate supports weak

[kinuko, 2012/11/20 06:25:33]

nit: fileapi::MTPDeviceDelegate -> 'the base class' might make it slightly
easier to follow (for me)

[kmadhusu, 2012/11/21 04:09:53]

Done.

+// pointers.
+class MTPDeviceDelegateImplLinux : 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.
+  // Should only be called by ScopedMTPDeviceMapEntry. Use
+  // GetAsWeakPtrOnIOThread() to get a weak pointer instance of this class.
+  // Defer the device initializations until the first file operation request.
+  // Do all the initializations in LazyInit() function.
   explicit MTPDeviceDelegateImplLinux(const std::string& device_location);
 
-  // Overridden from MTPDeviceDelegate. All the functions are called on
-  // |media_task_runner_|.
+  // MTPDeviceDelegate:

[kinuko, 2012/11/20 06:25:33]

nit: MTPDeviceDelegate overrides ?

[kmadhusu, 2012/11/21 04:09:53]

During MTPDeviceDelegateWin CL review, pkasting@ mentioned that I don't need to
specify the word "overrides" in the comment and he said it is fine to just say
"MTPDeviceDelegate:". So, I am following that format.

   virtual base::PlatformFileError GetFileInfo(
       const FilePath& file_path,
       base::PlatformFileInfo* file_info) OVERRIDE;
@@ -46,25 +49,27 @@ class MTPDeviceDelegateImplLinux : public fileapi::MTPDeviceDelegate,
       const FilePath& local_path,
       base::PlatformFileInfo* file_info) OVERRIDE;
   virtual base::SequencedTaskRunner* GetMediaTaskRunner() OVERRIDE;
-  virtual void DeleteOnCorrectThread() const OVERRIDE;
+  virtual void CancelPendingTasksAndDeleteDelegate() OVERRIDE;
+  virtual base::WeakPtr<fileapi::MTPDeviceDelegate> GetAsWeakPtrOnIOThread()
+      OVERRIDE;
 
  private:
-  friend struct fileapi::MTPDeviceDelegateDeleter;
-  friend class base::DeleteHelper<MTPDeviceDelegateImplLinux>;
-
-  // Private because this class is ref-counted.
+  // Destructed via DeleteDelegateOnTaskRunner(). Do all the clean up in
+  // DeleteDelegateOnTaskRunner().
   virtual ~MTPDeviceDelegateImplLinux();
 
-  // content::NotificationObserver implementation.
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
-
   // Opens the device for communication. This function is called on
   // |media_task_runner_|. Returns true if the device is ready for
   // communication, else false.
   bool LazyInit();
 
+  // Deletes the delegate on the task runner thread. This is called when the

[Lei Zhang, 2012/11/21 01:33:30]

The part that starts with "This is called" is a repeat of the comments in the
CancelPendingTasksAndDeleteDelegate() interface declaration. Just say something
like "Called by CancelPendingTasksAndDeleteDelegate(). Performs cleanup that
needs to happen on |media_task_runner_|.

[kmadhusu, 2012/11/21 04:09:53]

Done.

+  // browser application is in shutdown mode or when the last extension using
+  // this MTP device delegate is destroyed or when the MTP device is removed
+  // from the system or when an extension revokes the MTP device media gallery
+  // permissions.
+  void DeleteDelegateOnTaskRunner();
+
   // Stores the registered file system device path value. This path does not
   // correspond to a real device path (E.g.: "/usb:2,2:81282").
   const std::string device_path_;
@@ -78,17 +83,13 @@ class MTPDeviceDelegateImplLinux : public fileapi::MTPDeviceDelegate,
   scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
 
   // |media_task_runner_| can wait on this event until the requested task is
-  // complete.
+  // complete. Signaled on a different thread.

[Lei Zhang, 2012/11/21 01:33:30]

I think the comment is a given. If a thread is waiting, it can't be the one
signaling.

[kmadhusu, 2012/11/21 04:09:53]

Removed the comment.

   base::WaitableEvent on_task_completed_event_;
 
   // Used to notify |media_task_runner_| pending tasks about the shutdown
-  // sequence.
+  // sequence. Signaled on the IO thread.
   base::WaitableEvent on_shutdown_event_;
 
-  // Handles registering notifications with the NotificationService.
-  // Used to listen for application termination message.
-  content::NotificationRegistrar registrar_;
-
   DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplLinux);
 };