Media Gallery: Register isolated file systems on a per RenderProcessHost basis. Otherwise when one…

chrome/browser/media_gallery/media_file_system_registry.cc

Index: chrome/browser/media_gallery/media_file_system_registry.cc
===================================================================
--- chrome/browser/media_gallery/media_file_system_registry.cc	(revision 141331)
+++ chrome/browser/media_gallery/media_file_system_registry.cc	(working copy)
@@ -9,9 +9,11 @@
 #include <set>
 
 #include "base/path_service.h"
-#include "base/sys_string_conversions.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_source.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
 #include "webkit/fileapi/isolated_context.h"
 
 namespace chrome {
@@ -20,19 +22,41 @@
     g_media_file_system_registry = LAZY_INSTANCE_INITIALIZER;
 
 using content::BrowserThread;
+using content::RenderProcessHost;
 using fileapi::IsolatedContext;
 
+/******************
+ * Public methods
+ ******************/
+
 // static
 MediaFileSystemRegistry* MediaFileSystemRegistry::GetInstance() {
   return g_media_file_system_registry.Pointer();
 }
 
 std::vector<MediaFileSystemRegistry::MediaFSIDAndPath>
-MediaFileSystemRegistry::GetMediaFileSystems() const {
+MediaFileSystemRegistry::GetMediaFileSystems(
+    const content::RenderProcessHost* rph) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   std::vector<MediaFSIDAndPath> results;
-  for (MediaPathToFSIDMap::const_iterator it = media_fs_map_.begin();
-       it != media_fs_map_.end();
+  ChildIdToMediaFSMap::iterator child_it = media_fs_map_.find(rph);

[kinuko, 2012/06/11 12:21:06]

Can we simply call insert() here and check the second of the returned value to
see if it was succeeded (therefore we need to RegisterForRPH... etc)?

[Lei Zhang, 2012/06/11 19:59:07]

Done.

+  if (child_it == media_fs_map_.end()) {
+    // Never seen a GetMediaFileSystems call from this RPH. Initialize its
+    // file system mappings.
+    RegisterForRPHGoneNotifications(rph);
+    child_it = media_fs_map_.insert(child_it,
+                                    std::make_pair(rph, MediaPathToFSIDMap()));
+    FilePath pictures_path;
+    if (PathService::Get(chrome::DIR_USER_PICTURES, &pictures_path)) {
+      std::string fsid = RegisterPathAsFileSystem(pictures_path);
+      child_it->second.insert(std::make_pair(pictures_path, fsid));

[kinuko, 2012/06/11 12:21:06]

nit:
child_it->second[pictures_path] = fsid;
might be slightly easier to read?

[Lei Zhang, 2012/06/11 19:59:07]

Yes, but "foo[bar] = qux" is less efficient. My C++ readability reviewer said
google3 prefers insert() over operator[].

[kinuko, 2012/06/12 04:40:11]

Ok, in chromium review I was once told I should use foo[bar] over .insert for
readability, but I'm ok with either of them :)

+    }
+  }
+
+  MediaPathToFSIDMap& child_map = child_it->second;
+  for (MediaPathToFSIDMap::const_iterator it = child_map.begin();
+       it != child_map.end();
        ++it) {
     const FilePath path = it->first;
     const std::string fsid = it->second;
@@ -41,17 +65,50 @@
   return results;
 }
 
+void MediaFileSystemRegistry::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  DCHECK(type == content::NOTIFICATION_RENDERER_PROCESS_CLOSED ||
+         type == content::NOTIFICATION_RENDERER_PROCESS_TERMINATED);
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  const RenderProcessHost* rph =
+      content::Source<content::RenderProcessHost>(source).ptr();
+  ChildIdToMediaFSMap::iterator child_it = media_fs_map_.find(rph);
+  CHECK(child_it != media_fs_map_.end());
+  // No need to revoke the isolated file systems. The RPH will do that.
+  media_fs_map_.erase(child_it);
+  UnregisterForRPHGoneNotifications(rph);
+}
+
+/******************
+ * Private methods
+ ******************/
+
 MediaFileSystemRegistry::MediaFileSystemRegistry() {
-  FilePath pictures_path;
-  if (PathService::Get(chrome::DIR_USER_PICTURES, &pictures_path)) {
-    RegisterPathAsFileSystem(pictures_path);
-  }
 }
 
 MediaFileSystemRegistry::~MediaFileSystemRegistry() {
 }
 
-void MediaFileSystemRegistry::RegisterPathAsFileSystem(const FilePath& path) {
+void MediaFileSystemRegistry::RegisterForRPHGoneNotifications(
+    const content::RenderProcessHost* rph) {
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                 content::Source<RenderProcessHost>(rph));
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                 content::Source<RenderProcessHost>(rph));
+}
+
+void MediaFileSystemRegistry::UnregisterForRPHGoneNotifications(
+    const content::RenderProcessHost* rph) {
+  registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                    content::Source<RenderProcessHost>(rph));
+  registrar_.Remove(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                    content::Source<RenderProcessHost>(rph));
+}
+
+std::string MediaFileSystemRegistry::RegisterPathAsFileSystem(
+    const FilePath& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // Sanity checks for |path|.
@@ -67,7 +124,7 @@
   const std::string fsid =
       IsolatedContext::GetInstance()->RegisterIsolatedFileSystem(fileset);
   CHECK(!fsid.empty());
-  media_fs_map_.insert(std::make_pair(path, fsid));
+  return fsid;
 }
 
 }  // namespace chrome