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

chrome/browser/media_gallery/media_file_system_registry.cc

 // 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.
 
 // MediaFileSystemRegistry implementation.
 
 #include "chrome/browser/media_gallery/media_file_system_registry.h"
 
 #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 {
 
 static base::LazyInstance<MediaFileSystemRegistry>::Leaky
     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;
     results.push_back(std::make_pair(fsid, path));
   }
   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|.
   CHECK(path.IsAbsolute());
   CHECK(!path.ReferencesParent());
   // Make sure |path| does not refer to '/' on Unix.
   // TODO(thestig) Check how BaseName() works for say, 'C:\' on Windows.
   CHECK(!path.BaseName().IsAbsolute());
   CHECK(!path.BaseName().empty());
 
   std::set<FilePath> fileset;
   fileset.insert(path);
   const std::string fsid =
       IsolatedContext::GetInstance()->RegisterIsolatedFileSystem(fileset);
   CHECK(!fsid.empty());
-  media_fs_map_.insert(std::make_pair(path, fsid));
+  return fsid;
 }
 
 }  // namespace chrome