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

content/browser/plugin_service.h

Index: content/browser/plugin_service.h
diff --git a/content/browser/plugin_service.h b/content/browser/plugin_service.h
index 2d2596e3d4e5e6173e28b55db512258c8f6c9615..89d7d01a46468cd12f08614c9ccf40c52bd521b5 100644
--- a/content/browser/plugin_service.h
+++ b/content/browser/plugin_service.h
@@ -13,6 +13,7 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/callback.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/waitable_event_watcher.h"
@@ -25,6 +26,7 @@
 #include "content/common/notification_registrar.h"
 #include "googleurl/src/gurl.h"
 #include "ipc/ipc_channel_handle.h"
+#include "webkit/plugins/npapi/plugin_group.h"

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

nit: can you get away with forward declaring?

 #include "webkit/plugins/webplugininfo.h"
 
 #if defined(OS_WIN)
@@ -39,13 +41,26 @@
 struct PepperPluginInfo;
 class PluginDirWatcherDelegate;
 
+namespace base {
+class MessageLoopProxy;
+}
+
 namespace content {
 class ResourceContext;
 class PluginServiceFilter;
 }
 
+namespace webkit {
+namespace npapi {
+class MockPluginList;
+class PluginList;
+}
+}
+
 // This must be created on the main thread but it's only called on the IO/file
-// thread.
+// thread. This is an asynchronous wrapper around the PluginList interface for
+// querying plugin information, and this should instead of that class in the
+// content layer and above.

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

above->below? (see http://dev.chromium.org/developers/content-module)

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

I guess it depends if you're looking up or down ;). Got suggestions for better
wording?

[jam, 2011/09/21 17:20:15]

(big nit!) It's been my experience that we use down, for example that's why i
made the diagram in the wiki above that way. A rough check with a few others
here confirmed that that's our convention.

 class CONTENT_EXPORT PluginService
     : public base::WaitableEventWatcher::Delegate,
       public NotificationObserver {
@@ -57,6 +72,11 @@ class CONTENT_EXPORT PluginService
     webkit::WebPluginInfo plugin;
   };
 
+  typedef base::Callback<void(std::vector<webkit::WebPluginInfo>)>

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

const ref (and below)

+      GetPluginsCallback;
+  typedef base::Callback<void(std::vector<webkit::npapi::PluginGroup>)>
+      GetPluginGroupsCallback;
+
   // Returns the PluginService singleton.
   static PluginService* GetInstance();
 
@@ -115,10 +135,24 @@ class CONTENT_EXPORT PluginService
                      webkit::WebPluginInfo* info,
                      std::string* actual_mime_type);
 
-  // Returns a list of all plug-ins available to the resource context. Must be
-  // called on the FILE thread.
+  // Marks the plugin list as dirty and will cause the plugins to be reloaded
+  // on the next access through GetPlugins() or GetPluginGroups().
+  void RefreshPluginList();
+
+  // Asynchronously loads plugins on the FILE thread and then calls back to the

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

nit: i think it's better not to mention file thread here, since on mac/win we'll
be going to a different process

+  // provided function on the calling MessageLoop on completion.
   void GetPlugins(const content::ResourceContext& context,
-                  std::vector<webkit::WebPluginInfo>* plugins);
+                  const GetPluginsCallback& callback);
+  // Variant of above, but does not perform filtering based on |context|.
+  void GetPlugins(const GetPluginsCallback& callback);
+
+  // Returns the list of cached plugin groups. This can be called from any
+  // thread, but note that the data can be stale.
+  void GetCachedPluginGroups(std::vector<webkit::npapi::PluginGroup>* groups);
+
+  // Gets the list of plugin groups, reloading them from disk if marked as dirty
+  // with RefreshPluginList().
+  void GetPluginGroups(const GetPluginGroupsCallback& callback);
 
   // Tells all the renderer processes to throw away their cache of the plugin
   // list, and optionally also reload all the pages with plugins.
@@ -129,6 +163,10 @@ class CONTENT_EXPORT PluginService
     filter_ = filter;
   }
 
+  // Sets the PluginList for testing environments. Be sure to change back to
+  // NULL at the end of the test!
+  void SetPluginListForTesting(webkit::npapi::MockPluginList* plugin_list);

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

nit: can you make this private and friend the test class, so we can be assured
that other code doesn't call it?

actually, since this is only used by the extension code, why did you move this
stuff here instead of keeping it where it was?

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

I personally hate friending tests in shared components. Since it takes a
MockPluginList, I think this is pretty safe. There's also a PRESUBMIT when you
check in code that calls a "ForTesting" method.
The extension code needs to "load" fake plugins, which is part of the PluginList
interface. Since the PluginService just uses the Singleton(), there needs to be
a way to inject. They previously did this just through the injector in their
code, but now that the extension code uses PluginService, the injector needed to
move here.

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

oh I didn't know about that presubmit check. I'm curious, why do you hate
friending tests?
so in the test, the extensions code calls PS. currently you call into PS from
the test to override the returned list. why not have the test call into the
extensions code to override the list, and if that's set, it doesn't even call
PS? it seems nicer if testing code is localized as much as possible to the code
that needs that override.

+
  private:
   friend struct DefaultSingletonTraits<PluginService>;
 
@@ -137,6 +175,9 @@ class CONTENT_EXPORT PluginService
   PluginService();
   virtual ~PluginService();
 
+  // Returns the PluginList, either the singleton or the override.
+  webkit::npapi::PluginList* GetPluginList();
+
   // base::WaitableEventWatcher::Delegate implementation.
   virtual void OnWaitableEventSignaled(base::WaitableEvent* waitable_event);
 
@@ -148,6 +189,11 @@ class CONTENT_EXPORT PluginService
 
   PepperPluginInfo* GetRegisteredPpapiPluginInfo(const FilePath& plugin_path);
 
+  // Function that is run on the FILE thread to load the plugins synchronously.
+  void GetPluginsInternal(const content::ResourceContext* context,
+                          base::MessageLoopProxy* target_loop,
+                          const GetPluginsCallback& callback);
+
   // Helper so we can do the plugin lookup on the FILE thread.
   void GetAllowedPluginForOpenChannelToPlugin(
       int render_process_id,
@@ -199,6 +245,10 @@ class CONTENT_EXPORT PluginService
 
   std::set<PluginProcessHost::Client*> pending_plugin_clients_;
 
+  // Alternate plugin list to use for testing environments. When NULL, defaults
+  // to the singleton.
+  webkit::npapi::PluginList* plugin_list_;
+
   DISALLOW_COPY_AND_ASSIGN(PluginService);
 };