Redesigned and refactored ScopedMTPDeviceMapEntry, MTPDeviceMapService & MTPDeviceDelegate classes.

chrome/browser/media_gallery/media_file_system_registry.cc

Index: chrome/browser/media_gallery/media_file_system_registry.cc
diff --git a/chrome/browser/media_gallery/media_file_system_registry.cc b/chrome/browser/media_gallery/media_file_system_registry.cc
index c97943dafa9d555e3807737842e9be7424b3c8fa..6bb8e54e5d227c8c1831b0ea6a772d2ac414d4c0 100644
--- a/chrome/browser/media_gallery/media_file_system_registry.cc
+++ b/chrome/browser/media_gallery/media_file_system_registry.cc
@@ -126,48 +126,36 @@ MediaFileSystemInfo::MediaFileSystemInfo(const std::string& fs_name,
 MediaFileSystemInfo::MediaFileSystemInfo() {}
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-// Class to manage MTPDeviceDelegateImpl object for the attached MTP device.
-// Refcounted to reuse the same MTP device delegate entry across extensions.
-// This class supports WeakPtr (extends SupportsWeakPtr) to expose itself as
-// a weak pointer to MediaFileSystemRegistry.
-class ScopedMTPDeviceMapEntry
-    : public base::RefCounted<ScopedMTPDeviceMapEntry>,
-      public base::SupportsWeakPtr<ScopedMTPDeviceMapEntry> {
+// Class to manage the lifetime of MTPDeviceDelegateImpl object for the
+// attached media transfer protocol (MTP) device. This object is constructed for
+// each MTP device.
+class ScopedMTPDeviceMapEntry {
  public:
-  // |no_references_callback| is called when the last ScopedMTPDeviceMapEntry
-  // reference goes away.
-  ScopedMTPDeviceMapEntry(const FilePath::StringType& device_location,
-                          const base::Closure& no_references_callback)
-      : device_location_(device_location),
-        no_references_callback_(no_references_callback) {
+  explicit ScopedMTPDeviceMapEntry(const FilePath::StringType& device_location)
+      : device_location_(device_location) {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         Bind(&MTPDeviceMapService::AddDelegate,
              base::Unretained(MTPDeviceMapService::GetInstance()),
              device_location_,
-             make_scoped_refptr(new MTPDeviceDelegateImpl(device_location))));
+             new MTPDeviceDelegateImpl(device_location)));
   }
 
- private:
-  // Friend declaration for ref counted implementation.
-  friend class base::RefCounted<ScopedMTPDeviceMapEntry>;
-
-  // Private because this class is ref-counted.
+  // Destructed when the last user of this MTP device is destroyed or when the
+  // MTP device is detached from the system or when the browser is in shutdown
+  // mode or when the extension revokes the MTP device gallery permissions.
   ~ScopedMTPDeviceMapEntry() {
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE,
         Bind(&MTPDeviceMapService::RemoveDelegate,
              base::Unretained(MTPDeviceMapService::GetInstance()),
              device_location_));
-    no_references_callback_.Run();
   }
 
-  // Store the MTP or PTP device location.
+ private:
+  // The MTP or PTP device location.
   const FilePath::StringType device_location_;
 
-  // A callback to call when the last reference of this object goes away.
-  base::Closure no_references_callback_;
-
   DISALLOW_COPY_AND_ASSIGN(ScopedMTPDeviceMapEntry);
 };
 #endif
@@ -240,10 +228,19 @@ class ExtensionGalleriesHost
     pref_id_map_.erase(gallery);
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-    MediaDeviceEntryReferencesMap::iterator mtp_device_host =
-        media_device_map_references_.find(id);
-    if (mtp_device_host != media_device_map_references_.end())
-      media_device_map_references_.erase(mtp_device_host);
+    for (MTPDeviceGalleryReferencesMap::iterator it =
+             mtp_device_gallery_references_.begin();
+         it != mtp_device_gallery_references_.end(); ++it) {
+      MediaGalleryPrefIdSet::iterator found = it->second.find(id);
+      if (found == it->second.end())
+        continue;
+      it->second.erase(id);
+      if (it->second.empty()) {
+        file_system_context_->RemoveMTPDeviceReferenceForHost(it->first, this);
+        mtp_device_gallery_references_.erase(it);
+      }
+      break;
+    }
 #endif
   }
 
@@ -274,9 +271,8 @@ class ExtensionGalleriesHost
   typedef std::map<MediaGalleryPrefId, MediaFileSystemInfo>
       PrefIdFsInfoMap;
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  typedef std::map<MediaGalleryPrefId,
-                   scoped_refptr<ScopedMTPDeviceMapEntry> >
-      MediaDeviceEntryReferencesMap;
+  typedef std::map<const FilePath::StringType, MediaGalleryPrefIdSet>
+      MTPDeviceGalleryReferencesMap;
 #endif
   typedef std::map<const RenderProcessHost*, std::set<const WebContents*> >
       RenderProcessHostRefCount;
@@ -289,7 +285,7 @@ class ExtensionGalleriesHost
     DCHECK(pref_id_map_.empty());
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-    DCHECK(media_device_map_references_.empty());
+    DCHECK(mtp_device_gallery_references_.empty());
 #endif
   }
 
@@ -358,11 +354,9 @@ class ExtensionGalleriesHost
             device_id, path);
       } else {
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-        scoped_refptr<ScopedMTPDeviceMapEntry> mtp_device_host;
         fsid = file_system_context_->RegisterFileSystemForMTPDevice(
-            device_id, path, &mtp_device_host);
-        DCHECK(mtp_device_host.get());
-        media_device_map_references_[pref_id] = mtp_device_host;
+            device_id, path, this);
+        mtp_device_gallery_references_[path.value()].insert(pref_id);
 #else
         NOTIMPLEMENTED();
         continue;
@@ -426,11 +420,14 @@ class ExtensionGalleriesHost
         file_system_context_->RevokeFileSystem(it->second.fsid);
       }
       pref_id_map_.clear();
-
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-      media_device_map_references_.clear();
+      for (MTPDeviceGalleryReferencesMap::iterator it =
+               mtp_device_gallery_references_.begin();
+           it != mtp_device_gallery_references_.end(); ++it) {
+        file_system_context_->RemoveMTPDeviceReferenceForHost(it->first, this);
+      }
+      mtp_device_gallery_references_.clear();
 #endif
-
       no_references_callback_.Run();
     }
   }
@@ -446,9 +443,9 @@ class ExtensionGalleriesHost
   PrefIdFsInfoMap pref_id_map_;
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-  // A map from the gallery preferences id to the corresponding media device
-  // host object.
-  MediaDeviceEntryReferencesMap media_device_map_references_;
+  // A map from the MTP device id to a set of media galleries that references to
+  // that device.
+  MTPDeviceGalleryReferencesMap mtp_device_gallery_references_;
 #endif
 
   // The set of render processes and web contents that may have references to
@@ -536,7 +533,6 @@ void MediaFileSystemRegistry::OnRemovableStorageAttached(
     const std::string& id, const string16& name,
     const FilePath::StringType& location) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
   if (!MediaStorageUtil::IsMediaDevice(id))
     return;
 
@@ -552,7 +548,6 @@ void MediaFileSystemRegistry::OnRemovableStorageAttached(
 void MediaFileSystemRegistry::OnRemovableStorageDetached(
     const std::string& id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
   // Since revoking a gallery in the ExtensionGalleriesHost may cause it
   // to be removed from the map and therefore invalidate any iterator pointing
   // to it, this code first copies all the invalid gallery ids and the
@@ -629,7 +624,7 @@ class MediaFileSystemRegistry::MediaFileSystemContextImpl
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
   virtual std::string RegisterFileSystemForMTPDevice(
       const std::string& device_id, const FilePath& path,
-      scoped_refptr<ScopedMTPDeviceMapEntry>* entry) {
+      const ExtensionGalleriesHost* galleries_host) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     DCHECK(!MediaStorageUtil::IsMassStorageDevice(device_id));
 
@@ -641,10 +636,17 @@ class MediaFileSystemRegistry::MediaFileSystemContextImpl
         IsolatedContext::GetInstance()->RegisterFileSystemForPath(
             fileapi::kFileSystemTypeDeviceMedia, path, &fs_name);
     CHECK(!fsid.empty());
-    DCHECK(entry);
-    *entry = registry_->GetOrCreateScopedMTPDeviceMapEntry(path.value());
+    registry_->AddGalleriesHostReferenceForMTPDevice(path.value(),
+                                                     galleries_host);
     return fsid;
   }
+
+  virtual void RemoveMTPDeviceReferenceForHost(
+      const FilePath::StringType& device_location,
+      const ExtensionGalleriesHost* galleries_host) {
+    registry_->RemoveGalleriesHostReferenceForMTPDevice(device_location,
+                                                        galleries_host);
+  }
 #endif
 
   virtual void RevokeFileSystem(const std::string& fsid) {
@@ -670,6 +672,8 @@ MediaFileSystemRegistry::~MediaFileSystemRegistry() {
   SystemMonitor* system_monitor = SystemMonitor::Get();
   if (system_monitor)
     system_monitor->RemoveDevicesChangedObserver(this);
+  DCHECK(mtp_device_delegate_map_.empty());
+  DCHECK(mtp_device_references_map_.empty());
 }
 
 void MediaFileSystemRegistry::OnPreferenceChanged(
@@ -712,27 +716,31 @@ void MediaFileSystemRegistry::OnPreferenceChanged(
 }
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-ScopedMTPDeviceMapEntry*
-MediaFileSystemRegistry::GetOrCreateScopedMTPDeviceMapEntry(
-    const FilePath::StringType& device_location) {
+void MediaFileSystemRegistry::AddGalleriesHostReferenceForMTPDevice(
+    const FilePath::StringType& device_location,
+    const ExtensionGalleriesHost* galleries_host) {
   MTPDeviceDelegateMap::iterator delegate_it =
-      mtp_delegate_map_.find(device_location);
-  if (delegate_it != mtp_delegate_map_.end() && delegate_it->second.get())
-    return delegate_it->second;
-  ScopedMTPDeviceMapEntry* mtp_device_host = new ScopedMTPDeviceMapEntry(
-      device_location, base::Bind(
-          &MediaFileSystemRegistry::RemoveScopedMTPDeviceMapEntry,
-          base::Unretained(this), device_location));
-  mtp_delegate_map_[device_location] = mtp_device_host->AsWeakPtr();
-  return mtp_device_host;
+      mtp_device_delegate_map_.find(device_location);
+  if (delegate_it == mtp_device_delegate_map_.end()) {
+    mtp_device_delegate_map_[device_location] =
+        new ScopedMTPDeviceMapEntry(device_location);
+  }
+  mtp_device_references_map_[device_location].insert(galleries_host);
 }
 
-void MediaFileSystemRegistry::RemoveScopedMTPDeviceMapEntry(
-    const FilePath::StringType& device_location) {
-  MTPDeviceDelegateMap::iterator delegate_it =
-      mtp_delegate_map_.find(device_location);
-  DCHECK(delegate_it != mtp_delegate_map_.end());
-  mtp_delegate_map_.erase(delegate_it);
+void MediaFileSystemRegistry::RemoveGalleriesHostReferenceForMTPDevice(
+    const FilePath::StringType& device_location,
+    const ExtensionGalleriesHost* galleries_host) {
+  MTPDeviceReferencesMap::iterator hosts_it =
+      mtp_device_references_map_.find(device_location);
+  DCHECK(hosts_it != mtp_device_references_map_.end());
+  DCHECK(hosts_it->second.find(galleries_host) != hosts_it->second.end());
+  hosts_it->second.erase(galleries_host);
+  if (hosts_it->second.empty()) {
+    delete mtp_device_delegate_map_[device_location];
+    mtp_device_delegate_map_.erase(device_location);
+    mtp_device_references_map_.erase(hosts_it);
+  }
 }
 #endif