Add PluginServiceFilter interface.

content/browser/content_browser_client.h

Index: content/browser/content_browser_client.h
diff --git a/content/browser/content_browser_client.h b/content/browser/content_browser_client.h
index 9983a219944ffa07946712a2e8a19a1e697c1f69..77ea549a6162eac5cd30daa84b5c211df2f0bacf 100644
--- a/content/browser/content_browser_client.h
+++ b/content/browser/content_browser_client.h
@@ -10,6 +10,7 @@
 
 #include "base/callback_old.h"
 #include "base/memory/weak_ptr.h"
+#include "content/browser/plugin_service.h"
 #include "content/common/content_client.h"
 #include "content/common/window_container_type.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebNotificationPresenter.h"
@@ -52,6 +53,12 @@ namespace ui {
 class Clipboard;
 }
 
+namespace webkit {
+namespace npapi {
+struct WebPluginInfo;
+}
+}
+
 namespace content {
 
 class BrowserContext;
@@ -151,6 +158,15 @@ class ContentBrowserClient {
   virtual bool AllowSaveLocalState(
       const content::ResourceContext& context) = 0;
 
+  // Creates a new PluginFilter.
+  // This is called on the IO thread.
+  virtual PluginService::Filter* CreatePluginFilter(

[jam, 2011/08/01 03:25:44]

in general, it's best to keep the embedder interface to a minimum. Since we
already have an interface to talk to the embedder in this case (i.e. Filter), we
can make the embedder responsible for registering the filter at startup (i.e. in
BrowserProcessImpl::CreateIOThread() where we force the PluginService to be
created in the call to PluginService::GetInstance()).

also, there doesn't seem to be a need to create a filter each time we need to
call GetPlugins, so can we just have one filter across the lifetime of the
browser process? that way, callers in the content module don't have to ask the
embedder for the filter before they call another content class (PluginService)

once you do all that, then PluginService should be able to handle not having a
filter. that way embedders that don't need this extra functionality don't need
to implement an empty filter (the easier we can make it for a new embedder, the
better)

[Bernhard Bauer, 2011/08/01 16:06:12]

There are two reasons for having a separate filter class. The first is that the
filtering callback isn't completely stateless at the moment: Whether we use the
default plug-in depends on whether we have seen a disabled plug-in before that
(see
http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...).
I'd like to change that at some point, but it requires rewriting the default
plug-in, and I don't want to block this refactoring on that.

The second reason is that I need to get the per-profile object which stores
plug-in information (PluginPrefs) from the ResourceContext (via ProfileIOData),
but ResourceContext can only be used on the IO thread, so I need to fetch it
there, and then pass it to the FILE thread.

If we want to make the interface for other embedders easier, we could just check
for a NULL PluginFilter and assume it allows all plugins (like the DummyFilter
does).

[jam, 2011/08/01 17:25:04]

You can move the "plugin->path.value() ==
webkit::npapi::kDefaultPluginLibraryName" check to PluginService.
I'm probably missing something, but I don't find "PluginPrefs" in this patch,
and I don't see the ResourceContext that's passed to the filter being used?

[Bernhard Bauer, 2011/08/01 18:07:39]

But PluginService doesn't even get to see disabled plug-ins, so it can't
distinguish between a disabled plug-in and a not installed one :-/
Sorry, that's in http://codereview.chromium.org/7477044/ :-) I just wanted to
already add all the information we're going to need later to the interface.

[jam, 2011/08/02 04:18:18]

I see. this can be accomplished in other ways than creating a filter each time.

do any of the callers to PluginService need to get more than one plugin back? if
not, then the whole vector can be passed to the filter in one function call, and
it can return an index as to which one to use. if we need to return multiple,
then perhaps also take a parameter that's a vector of ints.
It's cumbersome for everyone who wants to get the plugins on the file (or even
UI) thread to always have to go to the IO thread first to get PluginPrefs. The
changes to RMF are an example of the manual thread hopping that this adds, that
I think would be best to avoid. To me, this is an indication that PluginPrefs
shouldn't be tied to ResourceContext. Perhaps PluginPrefs can have a map from
ResourceContext->PluginPrefs, a map that's safe to use on any thread. Then the
chrome filter can get to the pref on the file thread.

[Bernhard Bauer, 2011/08/02 12:45:08]

I found a different way, by removing the default plug-in *before*
filtering.
But then I'd need something which keeps track of all existing
|ResourceContext|s :-/

I'd like to think of it the other way around: All clients of
PluginService are already on the IO or UI thread (and on the UI thread
they can get PluginPrefs via a ProfileKeyedServiceFactory), so they need
to jump to the FILE thread anyway. This just makes the thread hopping in
RenderMessageFilter explicit, and only there.

[jam, 2011/08/02 15:13:32]

I wouldn't say all clients, RenderMessageFilter uses it on the file thread.
Going forward, I can only imagine we'd have more client on the file thread, and
this requires them all to do manual thread hopping.
I'm not sure what you mean by more explicit. It adds a lot of manual work. i.e.
I don't think any of the changes to RMF are necessary. Part of the reason we
have BrowserMessageFilter is that it allows easy dispatching of messages to
different threads. It also helps catch certain bad patterns (i.e. like
deadlocks). Dispatching messages manually to different threads is something we
should avoid.

[Bernhard Bauer, 2011/08/02 15:52:47]

But only because it already jumps to the FILE thread in BrowserMessageFilter ;-)
The problem is that almost any object which I can use to get a handle to
PluginPrefs lives either on the IO thread or the UI thread, so if I directly
dispatch the message on the FILE thread, I can't use any of those.

Is your objection specifically about the thread jumping in RenderMessageFilter?
If so, maybe we could add some kind of asynchronous interface to PluginService,
to make it usable from any thread? We might even be able to unify that with the
PluginServiceFilter in some way.

[Bernhard Bauer, 2011/08/02 16:13:31]

Elaborating a bit more, we could have an abstract class PluginService::Client to
which the plug-ins are passed, and to which the content client could add filters
(like IPC::Channel::Receiver and MessageFilter). We could use that interface for
OpenChannelToPlugin as well then, for example.

Does that sound completely braindead?

+      int render_process_id,
+      int render_view_id,
+      const content::ResourceContext& resource_context,
+      const GURL& url,
+      const GURL& policy_url) = 0;
+
   // Allows the embedder to override the request context based on the URL for
   // certain operations, like cookie access. Returns NULL to indicate the
   // regular request context should be used.