[Media Galleries] Lazily initialize the storage monitor.

chrome/browser/media_galleries/media_file_system_registry.cc

Index: chrome/browser/media_galleries/media_file_system_registry.cc
diff --git a/chrome/browser/media_galleries/media_file_system_registry.cc b/chrome/browser/media_galleries/media_file_system_registry.cc
index e659ad7431d11df61bf904f0e98077d6a3cead7e..87872ff18cd8e53d89ae8da460d5b5683b6331c0 100644
--- a/chrome/browser/media_galleries/media_file_system_registry.cc
+++ b/chrome/browser/media_galleries/media_file_system_registry.cc
@@ -435,7 +435,18 @@ void MediaFileSystemRegistry::GetMediaFileSystemsForExtension(
 
   Profile* profile =
       Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext());
-  MediaGalleriesPreferences* preferences = GetPreferences(profile);
+  GetPreferencesAsync(
+      profile, base::Bind(
+          &MediaFileSystemRegistry::GetMediaFileSystemsPostStorageMonitorInit,
+          weak_ptr_factory_.GetWeakPtr(),
+          rvh, extension, callback));
+}
+
+void MediaFileSystemRegistry::GetMediaFileSystemsPostStorageMonitorInit(
+    const content::RenderViewHost* rvh,
+    const extensions::Extension* extension,
+    const MediaFileSystemsCallback& callback,
+    MediaGalleriesPreferences* preferences) {
   MediaGalleryPrefIdSet galleries =
       preferences->GalleriesForExtension(*extension);
 
@@ -444,6 +455,8 @@ void MediaFileSystemRegistry::GetMediaFileSystemsForExtension(
     return;
   }
 
+  Profile* profile =
+      Profile::FromBrowserContext(rvh->GetProcess()->GetBrowserContext());
   if (!ContainsKey(pref_change_registrar_map_, profile)) {
     PrefChangeRegistrar* pref_registrar = new PrefChangeRegistrar;
     pref_registrar->Init(profile->GetPrefs());
@@ -470,6 +483,36 @@ void MediaFileSystemRegistry::GetMediaFileSystemsForExtension(
                                       callback);
 }
 
+void MediaFileSystemRegistry::GetPreferencesAsync(
+    Profile* profile,
+    base::Callback<void(MediaGalleriesPreferences*)> callback) {
+  StorageMonitor* monitor = StorageMonitor::GetInstance();
+  // TODO(gbillock): Make sure tests all have a test storage monitor
+  // and eliminate the checks here and elsewhere.
+  if (!monitor) {
+    callback.Run(MediaGalleriesPreferencesFactory::GetForProfile(profile));
+    return;
+  }
+
+  if (monitor->IsInitialized()) {

[vandebo (ex-Chrome), 2013/05/16 18:56:27]

You don't need this check - StorageMonitor::Initialize does this internally.

[Greg Billock, 2013/05/16 23:27:55]

This is the fast path -- if its initialized we just immediately run the callback
in the same stack. If it isn't initialized, we call initialize() below.

[vandebo (ex-Chrome), 2013/05/17 22:19:58]

But it's not substantially faster than just calling into monitor->Initialize. 
If it's already initialized, it does almost exactly this line.

[Greg Billock, 2013/05/18 00:02:26]

It goes through a pair of callbacks. That's not crazy overhead, but it's worth
smoothing over. Or are you worried about code coming to depend on being called
in the same stack when it should not?

[vandebo (ex-Chrome), 2013/05/20 15:45:46]

There's one (not two) extra layers of callback, yes.  But If that simplifies the
code and lets us get rid of IsInitialized, then I think it's worthwhile.

+    callback.Run(GetPreferences(profile));
+    return;
+  }
+
+  monitor->Initialize(
+      base::Bind(&MediaFileSystemRegistry::OnStorageMonitorInitialized,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 profile, callback));
+
+  // TODO(gbillock): set a timeout here to bail out of init?
+}
+
+void MediaFileSystemRegistry::OnStorageMonitorInitialized(
+    Profile* profile,
+    base::Callback<void(MediaGalleriesPreferences*)> callback) {
+  callback.Run(GetPreferences(profile));

[vandebo (ex-Chrome), 2013/05/16 18:56:27]

I don't think this is safe.  If it is, a comment why should be added.  What
happens if profile goes away why StorageMonitor is initializing.

[Greg Billock, 2013/05/16 23:27:55]

Makes sense. It turns out all we do with this is call the factory GetForProfile
method. That'll return NULL if the profile is dead, but this happening at
runtime is considered a boo-boo. We need to coordinate with the factory, which
gets lifecycle calls per-profile.

[vandebo (ex-Chrome), 2013/05/17 22:19:58]

I think we need to handle this before committing anything.

[Greg Billock, 2013/05/18 00:02:26]

I'm pondering this, but coming up with nothing except to change the structure of
the async step pretty dramatically, and have callers get the preferences
themselves after the call. Managing profile life within this operation is the
kind of chore that's going to be a big headache and gum up the works.

+}
+
 MediaGalleriesPreferences* MediaFileSystemRegistry::GetPreferences(
     Profile* profile) {
   MediaGalleriesPreferences* preferences =
@@ -486,6 +529,7 @@ MediaGalleriesPreferences* MediaFileSystemRegistry::GetPreferences(
   StorageMonitor* monitor = StorageMonitor::GetInstance();
   if (!monitor)
     return preferences;
+  DCHECK(monitor->IsInitialized());
   std::vector<StorageInfo> existing_devices = monitor->GetAttachedStorage();
   for (size_t i = 0; i < existing_devices.size(); i++) {
     if (!MediaStorageUtil::IsMediaDevice(existing_devices[i].device_id))
@@ -632,7 +676,8 @@ class MediaFileSystemRegistry::MediaFileSystemContextImpl
 };
 
 MediaFileSystemRegistry::MediaFileSystemRegistry()
-    : file_system_context_(new MediaFileSystemContextImpl(this)) {
+    : file_system_context_(new MediaFileSystemContextImpl(this)),
+      weak_ptr_factory_(this) {
   // StorageMonitor may be NULL in unit tests.
   StorageMonitor* monitor = StorageMonitor::GetInstance();
   if (monitor)