MediaFileSystemRegistry unit tests.

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..1f064a1766fdfea524fe5dd3cb802d05b49841f5 100644
--- a/chrome/browser/media_gallery/media_file_system_registry.cc
+++ b/chrome/browser/media_gallery/media_file_system_registry.cc
@@ -126,51 +126,31 @@ 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> {
- 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) {
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        Bind(&MTPDeviceMapService::AddDelegate,
-             base::Unretained(MTPDeviceMapService::GetInstance()),
-             device_location_,
-             make_scoped_refptr(new MTPDeviceDelegateImpl(device_location))));
-  }
-
- private:
-  // Friend declaration for ref counted implementation.
-  friend class base::RefCounted<ScopedMTPDeviceMapEntry>;
-
-  // Private because this class is ref-counted.
-  ~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.
-  const FilePath::StringType device_location_;
+ScopedMTPDeviceMapEntry::ScopedMTPDeviceMapEntry(
+    const FilePath::StringType& device_location,
+    const base::Closure& no_references_callback)
+    : device_location_(device_location),
+      delegate_(new MTPDeviceDelegateImpl(device_location)),
+      no_references_callback_(no_references_callback) {
+  DCHECK(delegate_);

[Lei Zhang, 2012/11/09 09:14:53]

No need to check for OOM conditions

[vandebo (ex-Chrome), 2012/11/13 06:50:18]

Done.

+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&MTPDeviceMapService::AddDelegate,
+           base::Unretained(MTPDeviceMapService::GetInstance()),
+           device_location_, make_scoped_refptr(delegate_)));
+}
 
-  // A callback to call when the last reference of this object goes away.
-  base::Closure no_references_callback_;
+ScopedMTPDeviceMapEntry::~ScopedMTPDeviceMapEntry() {
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&MTPDeviceMapService::RemoveDelegate,
+           base::Unretained(MTPDeviceMapService::GetInstance()),
+           device_location_));
+  no_references_callback_.Run();
+}
 
-  DISALLOW_COPY_AND_ASSIGN(ScopedMTPDeviceMapEntry);
-};
-#endif
+#endif  // defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
 
 // The main owner of this class is
 // |MediaFileSystemRegistry::extension_hosts_map_|, but a callback may
@@ -245,6 +225,11 @@ class ExtensionGalleriesHost
     if (mtp_device_host != media_device_map_references_.end())
       media_device_map_references_.erase(mtp_device_host);
 #endif
+
+    if (pref_id_map_.empty()) {
+      rph_refs_.clear();
+      CleanUp();
+    }
   }
 
   // Indicate that the passed |rvh| will reference the file system ids created
@@ -265,7 +250,7 @@ class ExtensionGalleriesHost
     RenderProcessHost* rph = contents->GetRenderProcessHost();
     rph_refs_[rph].insert(contents);
     if (rph_refs_[rph].size() == 1) {
-      registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+      registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                      content::Source<RenderProcessHost>(rph));
     }
   }
@@ -298,8 +283,8 @@ class ExtensionGalleriesHost
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE {
     switch (type) {
-      case content::NOTIFICATION_RENDERER_PROCESS_CLOSED: {
-        OnRendererProcessClosed(
+      case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {
+        OnRendererProcessTerminated(
             content::Source<RenderProcessHost>(source).ptr());
         break;
       }
@@ -381,12 +366,17 @@ class ExtensionGalleriesHost
       pref_id_map_[pref_id] = new_entry;
     }
 
-    RevokeOldGalleries(new_galleries);
+    if (result.size() == 0) {
+      rph_refs_.clear();
+      CleanUp();
+    } else {
+      RevokeOldGalleries(new_galleries);
+    }
 
     callback.Run(result);
   }
 
-  void OnRendererProcessClosed(const RenderProcessHost* rph) {
+  void OnRendererProcessTerminated(const RenderProcessHost* rph) {
     RenderProcessHostRefCount::const_iterator rph_info = rph_refs_.find(rph);
     DCHECK(rph_info != rph_refs_.end());
     // We're going to remove everything from the set, so we make a copy
@@ -414,25 +404,31 @@ class ExtensionGalleriesHost
     DCHECK(process_refs != rph_refs_.end());
     process_refs->second.erase(contents);
     if (process_refs->second.empty()) {
-      registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+      registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                         content::Source<RenderProcessHost>(rph));
       rph_refs_.erase(process_refs);
     }
 
-    if (rph_refs_.empty()) {
-      for (PrefIdFsInfoMap::const_iterator it = pref_id_map_.begin();
-           it != pref_id_map_.end();
-           ++it) {
-        file_system_context_->RevokeFileSystem(it->second.fsid);
-      }
-      pref_id_map_.clear();
+    if (rph_refs_.empty())
+      CleanUp();
+  }
+
+  void CleanUp() {
+    DCHECK(rph_refs_.empty());
+    for (PrefIdFsInfoMap::const_iterator it = pref_id_map_.begin();
+         it != pref_id_map_.end();
+         ++it) {
+      file_system_context_->RevokeFileSystem(it->second.fsid);
+    }
+    pref_id_map_.clear();
 
 #if defined(SUPPORT_MTP_DEVICE_FILESYSTEM)
-      media_device_map_references_.clear();
+    media_device_map_references_.clear();
 #endif
 
-      no_references_callback_.Run();
-    }
+    registrar_.RemoveAll();
+
+    no_references_callback_.Run();
   }
 
   // MediaFileSystemRegistry owns |this| and |file_system_context_|, so it's
@@ -549,6 +545,10 @@ void MediaFileSystemRegistry::OnRemovableStorageAttached(
   }
 }
 
+size_t MediaFileSystemRegistry::GetExtensionHostCountForTests() {
+  return extension_hosts_map_.size();
+}
+
 void MediaFileSystemRegistry::OnRemovableStorageDetached(
     const std::string& id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));