Media Galleries: Always call MediaFileSystemRegistry::GetMediaFileSystemsForExtension() before show…

chrome/browser/extensions/api/media_galleries/media_galleries_api.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.
 
 // Implements the Chrome Extensions Media Galleries API.
 
 #include "chrome/browser/extensions/api/media_galleries/media_galleries_api.h"
 
 #include <set>
 #include <string>
@@ skipping 20 matching lines @@
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 
 #if defined(OS_WIN)
 #include "base/sys_string_conversions.h"
 #endif
 
 using chrome::MediaFileSystemInfo;
 using chrome::MediaFileSystemRegistry;
+using chrome::MediaFileSystemsCallback;
 using content::ChildProcessSecurityPolicy;
 using content::WebContents;
 
 namespace MediaGalleries = extensions::api::media_galleries;
 namespace GetMediaFileSystems = MediaGalleries::GetMediaFileSystems;
 
 namespace extensions {
 
 namespace {
 
@@ skipping 29 matching lines @@
       GetMediaFileSystems::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
   MediaGalleries::GetMediaFileSystemsInteractivity interactive =
       MediaGalleries::GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_NO;
   if (params->details.get() && params->details->interactive != MediaGalleries::
          GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_NONE) {
     interactive = params->details->interactive;
   }
 
   switch (interactive) {
-    case MediaGalleries::GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_YES:
-      ShowDialog();
+    case MediaGalleries::GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_YES: {
+      // On the first call to this API, the MediaFileSystemRegistry would not

[vandebo (ex-Chrome), 2013/01/30 21:24:25]

This comment is pretty opaque to the actual reason... How about something like

The DialogController uses preferences changes to update itself if the gallery
list changes when it is open.  To ensure that MediaFileSystemRegistry updates
the preferences, we need to call GetMediaFileSystemsForExtension so that it
knows that something is interested in updates for |GetExtension()|.

[Lei Zhang, 2013/01/30 23:13:34]

Done. I made it opaque because the statements in the suggested comments are not
yet true. But I suppose they will be soon.

+      // know about the calling extension if this code path simply shows the
+      // media gallery dialog. The GetMediaFileSystemsForExtension() call here
+      // lets the MediaFileSystemRegistry know the calling extension is
+      // interested in getting media galleries, even though AlwaysShowDialog()
+      // does not care about the returned results.
+      GetMediaFileSystemsForExtension(base::Bind(
+          &MediaGalleriesGetMediaFileSystemsFunction::AlwaysShowDialog, this));
       return true;
+    }
     case MediaGalleries::GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_IF_NEEDED: {
-      MediaFileSystemRegistry* registry =
-          g_browser_process->media_file_system_registry();
-      registry->GetMediaFileSystemsForExtension(
-          render_view_host(), GetExtension(), base::Bind(
-              &MediaGalleriesGetMediaFileSystemsFunction::
-                  ShowDialogIfNoGalleries,
-              this));
+      GetMediaFileSystemsForExtension(base::Bind(
+          &MediaGalleriesGetMediaFileSystemsFunction::ShowDialogIfNoGalleries,
+          this));
       return true;
     }
     case MediaGalleries::GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_NO:
       GetAndReturnGalleries();
       return true;
     case MediaGalleries::GET_MEDIA_FILE_SYSTEMS_INTERACTIVITY_NONE:
       NOTREACHED();
   }
   error_ = kInvalidInteractive;
   return false;
 }
 
+void MediaGalleriesGetMediaFileSystemsFunction::AlwaysShowDialog(

[vandebo (ex-Chrome), 2013/01/30 21:24:25]

Hmm, I wonder if it would be worthwhile to add a bind helper that would eat an
argument, so you could just use that at the call site...

+    const std::vector<MediaFileSystemInfo>& /*filesystems*/) {
+  ShowDialog();
+}
+
 void MediaGalleriesGetMediaFileSystemsFunction::ShowDialogIfNoGalleries(
     const std::vector<MediaFileSystemInfo>& filesystems) {
   if (filesystems.empty())
     ShowDialog();
   else
     ReturnGalleries(filesystems);
 }
 
 void MediaGalleriesGetMediaFileSystemsFunction::GetAndReturnGalleries() {
-  MediaFileSystemRegistry* registry =
-      g_browser_process->media_file_system_registry();
-  registry->GetMediaFileSystemsForExtension(
-      render_view_host(), GetExtension(), base::Bind(
-          &MediaGalleriesGetMediaFileSystemsFunction::ReturnGalleries, this));
+  GetMediaFileSystemsForExtension(base::Bind(
+      &MediaGalleriesGetMediaFileSystemsFunction::ReturnGalleries, this));
 }
 
 void MediaGalleriesGetMediaFileSystemsFunction::ReturnGalleries(
     const std::vector<MediaFileSystemInfo>& filesystems) {
   content::RenderViewHost* rvh = render_view_host();
   if (!rvh) {
     SendResponse(false);
     return;
   }
   MediaGalleriesPermission::CheckParam read_param(
@@ skipping 77 matching lines @@
       return;
     }
   }
 
   // Controller will delete itself.
   base::Closure cb = base::Bind(
       &MediaGalleriesGetMediaFileSystemsFunction::GetAndReturnGalleries, this);
   new chrome::MediaGalleriesDialogController(contents, *GetExtension(), cb);
 }
 
+void MediaGalleriesGetMediaFileSystemsFunction::GetMediaFileSystemsForExtension(
+    const chrome::MediaFileSystemsCallback& cb) {
+  MediaFileSystemRegistry* registry =
+      g_browser_process->media_file_system_registry();
+  registry->GetMediaFileSystemsForExtension(
+      render_view_host(), GetExtension(), cb);
+}
+
 // MediaGalleriesAssembleMediaFileFunction -------------------------------------
 
 MediaGalleriesAssembleMediaFileFunction::
     ~MediaGalleriesAssembleMediaFileFunction() {}
 
 bool MediaGalleriesAssembleMediaFileFunction::RunImpl() {
   if (!ApiIsAccessible(&error_))
     return false;
 
   // TODO(vandebo) Update the metadata and return the new file.
   SetResult(base::Value::CreateNullValue());
   return true;
 }
 
 }  // namespace extensions