Convert the PluginService interface to be an async wrapper around PluginList.

content/browser/plugin_service.cc

Index: content/browser/plugin_service.cc
diff --git a/content/browser/plugin_service.cc b/content/browser/plugin_service.cc
index 0a6007cca5605a32918307d332004e418edaef85..410de61be5d572b4a4c5c80daa71de59f063d250 100644
--- a/content/browser/plugin_service.cc
+++ b/content/browser/plugin_service.cc
@@ -4,9 +4,12 @@
 
 #include "content/browser/plugin_service.h"
 
+#include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
+#include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/synchronization/waitable_event.h"
@@ -26,6 +29,7 @@
 #include "content/common/pepper_plugin_registry.h"
 #include "content/common/plugin_messages.h"
 #include "content/common/view_messages.h"
+#include "webkit/plugins/npapi/mock_plugin_list.h"

[Bernhard Bauer, 2011/09/21 14:31:26]

Nit: Should PluginService really know about testing-specific code? You could
just as well pass in a regular PluginList to SetPluginListForTesting.

 #include "webkit/plugins/npapi/plugin_constants_win.h"
 #include "webkit/plugins/npapi/plugin_list.h"
 #include "webkit/plugins/webplugininfo.h"
@@ -36,6 +40,28 @@ using ::base::files::FilePathWatcher;
 
 using content::PluginServiceFilter;
 
+namespace {
+
+// Helper function that merely runs the callback with the result. Called on the
+// thread on which the original GetPlugins() call was made.
+static void RunGetPluginsCallback(
+    const PluginService::GetPluginsCallback& callback,
+    std::vector<webkit::WebPluginInfo> result) {
+  callback.Run(result);
+}
+
+// A callback for GetPlugins() that then gets the freshly loaded plugin groups
+// and runs the callback for GetPluginGroups().
+static void GetPluginsForGroupsCallback(
+    const PluginService::GetPluginGroupsCallback& callback,
+    std::vector<webkit::WebPluginInfo> /* unused */) {

[jam, 2011/09/21 00:04:51]

nit: const ref.

also, "unused" isn't a convention in any of the plugin classes for unused
parameters. i think it's clear to the reader that it's unused, so you can just
put infos

actually, why pass in webplugininfo if it's not used at all? and even, why not
just give it the PluginGroup objects as a parameter instead of calling PS?

[Robert Sesek, 2011/09/21 01:13:44]

As stated in the comments, this is the callback for the call to GetPlugins().
This is done so that the plugin cache is fresh and reloaded if necessary, after
which the plugin groups can be fetched directly from the cache.

+  std::vector<webkit::npapi::PluginGroup> groups;
+  PluginService::GetInstance()->GetCachedPluginGroups(&groups);
+  callback.Run(groups);
+}
+
+}  // namespace
+
 #if defined(OS_MACOSX)
 static void NotifyPluginsOfActivation() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ -70,7 +96,8 @@ PluginService* PluginService::GetInstance() {
 PluginService::PluginService()
     : ui_locale_(
           content::GetContentClient()->browser()->GetApplicationLocale()),
-      filter_(NULL) {
+      filter_(NULL),
+      plugin_list_(NULL) {
   RegisterPepperPlugins();
 
   // Load any specified on the command line as well.
@@ -134,8 +161,7 @@ void PluginService::StartWatchingPlugins() {
   // Get the list of all paths for registering the FilePathWatchers
   // that will track and if needed reload the list of plugins on runtime.
   std::vector<FilePath> plugin_dirs;
-  webkit::npapi::PluginList::Singleton()->GetPluginDirectories(
-      &plugin_dirs);
+  GetPluginList()->GetPluginDirectories(&plugin_dirs);
 
   for (size_t i = 0; i < plugin_dirs.size(); ++i) {
     // FilePathWatcher can not handle non-absolute paths under windows.
@@ -216,8 +242,7 @@ PluginProcessHost* PluginService::FindOrStartNpapiPluginProcess(
     return plugin_host;
 
   webkit::WebPluginInfo info;
-  if (!webkit::npapi::PluginList::Singleton()->GetPluginInfoByPath(
-          plugin_path, &info)) {
+  if (!GetPluginList()->GetPluginInfoByPath(plugin_path, &info)) {
     return NULL;
   }
 
@@ -384,8 +409,7 @@ bool PluginService::GetPluginInfo(int render_process_id,
                                   bool* use_stale,
                                   webkit::WebPluginInfo* info,
                                   std::string* actual_mime_type) {
-  webkit::npapi::PluginList* plugin_list =
-      webkit::npapi::PluginList::Singleton();
+  webkit::npapi::PluginList* plugin_list = GetPluginList();
   // GetPluginInfoArray may need to load the plugins, so we need to be
   // on the FILE thread.
   DCHECK(use_stale || BrowserThread::CurrentlyOn(BrowserThread::FILE));
@@ -417,31 +441,81 @@ bool PluginService::GetPluginInfo(int render_process_id,
   return false;
 }
 
-void PluginService::GetPlugins(
-    const content::ResourceContext& context,
-    std::vector<webkit::WebPluginInfo>* plugins) {
-  // GetPlugins may need to load the plugins, so we need to be
-  // on the FILE thread.
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
-  webkit::npapi::PluginList* plugin_list =
-      webkit::npapi::PluginList::Singleton();
+void PluginService::RefreshPluginList() {
+  GetPluginList()->RefreshPlugins();
+}
+
+void PluginService::GetPlugins(const content::ResourceContext& context,
+    const PluginService::GetPluginsCallback& callback) {
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+      base::Bind(&PluginService::GetPluginsInternal, base::Unretained(this),
+          base::Unretained(&context),

[jam, 2011/09/21 00:04:51]

how do you know that the ResourceContext will still be around when we go to the
file thread? it seems that this can cause crashes when profiles go away

[Robert Sesek, 2011/09/21 01:13:44]

I was concerned about this too, but was wondering the best approach. Should I
make the ResourceContext extend from SupportsWeakPtr<T>?

[jam, 2011/09/21 06:31:46]

You'd have to check with Will Chan about this. It seems like a workaround
because one is unsure about lifetime. also, weakptr is not threadsafe to use on
threads other than the one it was created on.

it looks like there's only one caller of this function (RMF), so perhaps the
best approach for now is to have only one GetPlugins(callback) call, and have
RMF do its filtering when it gets the result.

[Bernhard Bauer, 2011/09/21 14:31:26]

Yes please! Then we can do the filtering on the IO thread and get rid of the
resource context map in ChromePluginServiceFilter.

+          MessageLoop::current()->message_loop_proxy(),
+          callback));
+}
+
+void PluginService::GetPlugins(const GetPluginsCallback& callback) {
+  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
+      base::Bind(&PluginService::GetPluginsInternal, base::Unretained(this),
+          base::Unretained(static_cast<content::ResourceContext*>(NULL)),
+          MessageLoop::current()->message_loop_proxy(),
+          callback));
+}
+
+void PluginService::GetCachedPluginGroups(
+    std::vector<webkit::npapi::PluginGroup>* groups) {
+  GetPluginList()->GetPluginGroups(false, groups);
+}
+
+void PluginService::GetPluginGroups(const GetPluginGroupsCallback& callback) {
+  GetPlugins(base::Bind(&GetPluginsForGroupsCallback, callback));
+}
+
+void PluginService::GetPluginsInternal(const content::ResourceContext* context,
+     base::MessageLoopProxy* target_loop,
+     const PluginService::GetPluginsCallback& callback) {

[jam, 2011/09/21 00:04:51]

probably want to copy the
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE)); from line 425

[Robert Sesek, 2011/09/21 01:13:44]

No, this will be changing shortly anyways.

[jam, 2011/09/21 06:31:46]

On Windows this will still be in-process, so we still want the assert there to
ensure people don't do this heavy disk operation on UI/IO

+  // Load all the plugins synchronously.
+  webkit::npapi::PluginList* plugin_list = GetPluginList();
   std::vector<webkit::WebPluginInfo> all_plugins;
   plugin_list->GetPlugins(&all_plugins);
 
-  int child_process_id = -1;
-  int routing_id = MSG_ROUTING_NONE;
-  for (size_t i = 0; i < all_plugins.size(); ++i) {
-    if (!filter_ || filter_->ShouldUsePlugin(child_process_id,
-                                             routing_id,
-                                             &context,
-                                             GURL(),
-                                             GURL(),
-                                             &all_plugins[i])) {
-      plugins->push_back(all_plugins[i]);
+  if (context) {
+    // If there is a context, filter plugins.
+    std::vector<webkit::WebPluginInfo> plugins;
+
+    int child_process_id = -1;
+    int routing_id = MSG_ROUTING_NONE;
+    for (size_t i = 0; i < all_plugins.size(); ++i) {
+      if (!filter_ || filter_->ShouldUsePlugin(child_process_id,
+                                               routing_id,
+                                               context,
+                                               GURL(),
+                                               GURL(),
+                                               &all_plugins[i])) {
+        plugins.push_back(all_plugins[i]);
+      }
     }
+
+    target_loop->PostTask(FROM_HERE,
+        base::Bind(&RunGetPluginsCallback, callback, plugins));
+  } else {
+    // Otherwise, return all plugins.
+    target_loop->PostTask(FROM_HERE,
+        base::Bind(&RunGetPluginsCallback, callback, all_plugins));
   }
 }
 
+void PluginService::SetPluginListForTesting(
+    webkit::npapi::MockPluginList* plugin_list) {
+  plugin_list_ = plugin_list;
+}
+
+webkit::npapi::PluginList* PluginService::GetPluginList() {
+  if (plugin_list_)
+    return plugin_list_;
+  return webkit::npapi::PluginList::Singleton();
+}
+
 void PluginService::OnWaitableEventSignaled(
     base::WaitableEvent* waitable_event) {
 #if defined(OS_WIN)
@@ -451,7 +525,7 @@ void PluginService::OnWaitableEventSignaled(
     hklm_key_.StartWatching();
   }
 
-  webkit::npapi::PluginList::Singleton()->RefreshPlugins();
+  GetPluginList()->RefreshPlugins();
   PurgePluginListCache(true);
 #else
   // This event should only get signaled on a Windows machine.
@@ -483,7 +557,7 @@ void PluginService::RegisterPepperPlugins() {
   // TODO(abarth): It seems like the PepperPluginRegistry should do this work.
   PepperPluginRegistry::ComputeList(&ppapi_plugins_);
   for (size_t i = 0; i < ppapi_plugins_.size(); ++i) {
-    webkit::npapi::PluginList::Singleton()->RegisterInternalPlugin(
+    GetPluginList()->RegisterInternalPlugin(
         ppapi_plugins_[i].ToWebPluginInfo());
   }
 }
@@ -506,8 +580,7 @@ PepperPluginInfo* PluginService::GetRegisteredPpapiPluginInfo(
   // construct it and add it to the list. This same deal needs to be done
   // in the renderer side in PepperPluginRegistry.
   webkit::WebPluginInfo webplugin_info;
-  if (!webkit::npapi::PluginList::Singleton()->GetPluginInfoByPath(
-      plugin_path, &webplugin_info))
+  if (!GetPluginList()->GetPluginInfoByPath(plugin_path, &webplugin_info))
     return NULL;
   PepperPluginInfo new_pepper_info;
   if (!MakePepperPluginInfo(webplugin_info, &new_pepper_info))