Add PluginServiceFilter interface.

Add PluginServiceFilter interface and ChromePluginServiceFilter.

This should get rid of all references to webkit::npapi::IsPluginEnabled in content/.

BUG=80794
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99355

[Bernhard Bauer, 2011-07-29 15:56:04]

Please review :-)

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

I like making content simpler :) some comments

http://codereview.chromium.org/7387010/diff/30001/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_helper.h (right):

http://codereview.chromium.org/7387010/diff/30001/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.h:23: class
ChromePluginServiceHelper : public NotificationObserver {
we can make this also implement the filter interface (once it's not owned by
content). FindOverriddenPluginForTab() and indRestrictedURLForPlugin) can be
private then.

http://codereview.chromium.org/7387010/diff/30001/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.h:25: struct OverriddenPlugin {
while you're cleaning this up, what do you think of making OverridePlugin a
private struct that we don't need to expose, and OverridePluginForTab() should
just take its members directly?

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
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)

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
content/browser/plugin_service.h:48: class Filter {
please put this in a separate file. per the style guide, we should avoid nested
classes since they can't be forward declared and implementors/functions that use
it end up bring all of plugin_service.h  in their header

http://codereview.chromium.org/7387010/diff/30001/content/browser/renderer_ho...
File content/browser/renderer_host/render_message_filter.cc (left):

http://codereview.chromium.org/7387010/diff/30001/content/browser/renderer_ho...
content/browser/renderer_host/render_message_filter.cc:296: void
RenderMessageFilter::OverrideThreadForMessage(const IPC::Message& message,
i believe the changes to this file wouldn't be required once there's one Filter
for the lifetime of the browser

http://codereview.chromium.org/7387010/diff/30001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/common/view_message...
content/common/view_messages.h:1544: GURL /* policy_url */,
policy_url is a chrome concept, so it doesn't belong in content. since i see
this is coming from WebPluginDelegateProxy::page_url_, just keep it called that
instead

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

Thanks for the review!

http://codereview.chromium.org/7387010/diff/30001/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_helper.h (right):

http://codereview.chromium.org/7387010/diff/30001/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.h:25: struct OverriddenPlugin {
On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> while you're cleaning this up, what do you think of making OverridePlugin a
> private struct that we don't need to expose, and OverridePluginForTab() should
> just take its members directly?

Sounds good. Done.

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> 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)

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).

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
content/browser/plugin_service.h:48: class Filter {
On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> please put this in a separate file. per the style guide, we should avoid
nested
> classes since they can't be forward declared and implementors/functions that
use
> it end up bring all of plugin_service.h  in their header

Done (BTW, is there a rule for when we put content/ classes into a content::
namespace?).

http://codereview.chromium.org/7387010/diff/30001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/common/view_message...
content/common/view_messages.h:1544: GURL /* policy_url */,
On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> policy_url is a chrome concept, so it doesn't belong in content. since i see
> this is coming from WebPluginDelegateProxy::page_url_, just keep it called
that
> instead

Done.

[jam, 2011-08-01 17:25:03]

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > 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)
> 
> 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.

You can move the "plugin->path.value() ==
webkit::npapi::kDefaultPluginLibraryName" check to PluginService.

> 
> 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.

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?

> 
> 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).

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
content/browser/plugin_service.h:48: class Filter {
On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > please put this in a separate file. per the style guide, we should avoid
> nested
> > classes since they can't be forward declared and implementors/functions that
> use
> > it end up bring all of plugin_service.h  in their header
> 
> Done (BTW, is there a rule for when we put content/ classes into a content::
> namespace?).

everything in content should be in the content namespace, but we haven't started
converting existing classes. new classes, like this filter, should go into it

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_helper.cc (right):

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.cc:69: // TODO(bauerb): Implement
the default plug-in functionality directly
I don't understand this comment? Why do you think implementing the default
plugin functionality using a plugin is hacky?

[Bernhard Bauer, 2011-08-01 18:07:38]

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > 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)
> > 
> > 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.
> 
> You can move the "plugin->path.value() ==
> webkit::npapi::kDefaultPluginLibraryName" check to PluginService.

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 :-/

> 
> > 
> > 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.
> 
> 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?

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.

> > 
> > 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).
>

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/plugin_serv...
content/browser/plugin_service.h:48: class Filter {
On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > please put this in a separate file. per the style guide, we should avoid
> > nested
> > > classes since they can't be forward declared and implementors/functions
that
> > use
> > > it end up bring all of plugin_service.h  in their header
> > 
> > Done (BTW, is there a rule for when we put content/ classes into a content::
> > namespace?).
> 
> everything in content should be in the content namespace, but we haven't
started
> converting existing classes. new classes, like this filter, should go into it

OK, done.

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_helper.cc (right):

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.cc:69: // TODO(bauerb): Implement
the default plug-in functionality directly
On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> I don't understand this comment? Why do you think implementing the default
> plugin functionality using a plugin is hacky?

Using a plug-in that handles MIME type "*" is a nice idea, but we need to break
that abstraction in so many places (like here) that it's easier to just use a
placeholder like we have for blocked plug-ins.

[jam, 2011-08-02 04:18:17]

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/01 18:07:39, Bernhard Bauer wrote:
> On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > > 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)
> > > 
> > > 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.
> > 
> > You can move the "plugin->path.value() ==
> > webkit::npapi::kDefaultPluginLibraryName" check to PluginService.
> 
> 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 :-/

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.

> 
> > 
> > > 
> > > 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.
> > 
> > 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?
> 
> 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.


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.

> 
> > > 
> > > 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).
> > 
>

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_helper.cc (right):

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.cc:60: *plugin =
*overridden_plugin_;
why is this line needed? i.e. under what conditions will the paths match but
something else will be different?

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.cc:80: // Check whether the plugin
is restricted to a URL.
btw, now that this code lives in chrome, we can get rid of the "restricted
plugin for url" idiom and just call out to extensionsservice. that indirection
was added when this code was living in content

http://codereview.chromium.org/7387010/diff/39053/content/browser/plugin_filt...
File content/browser/plugin_filter.h (right):

http://codereview.chromium.org/7387010/diff/39053/content/browser/plugin_filt...
content/browser/plugin_filter.h:13: class PluginFilter {
nit: i think naming this PluginServiceFilter makes it clear what it's used for

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

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> On 2011/08/01 18:07:39, Bernhard Bauer wrote:
> > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > > > 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 havinga 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)
> > > >
> > > > 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.
> > >
> > > You can move the "plugin->path.value() ==
> > > webkit::npapi::kDefaultPluginLibraryName" check to PluginService.
> >
> > 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
> > :-/
>
> 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.

I found a different way, by removing the default plug-in *before*
filtering.

> > > > 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.
> > >
> > > 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?
> >
> > 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.
>
>
> 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.

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.

> > > > 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).
> >
> >

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_helper.cc (right):

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.cc:60: *plugin =
*overridden_plugin_;
On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> why is this line needed? i.e. under what conditions will the paths match but
> something else will be different?

When we want to use an out-of-date version of Reader to open a PDF (from the
infobar), we return a WebPluginInfo with a different version, so it's not
blocked in the renderer.

http://codereview.chromium.org/7387010/diff/36002/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_helper.cc:80: // Check whether the plugin
is restricted to a URL.
On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> btw, now that this code lives in chrome, we can get rid of the "restricted
> plugin for url" idiom and just call out to extensionsservice. that indirection
> was added when this code was living in content

Hmm, ExtensionService currently lives on the UI thread, so I'd need to make it
thread-safe in that regard. 

Also, I'd need to get the ExtensionService for the current profile, and all of
that actually lives on the UI thread :-/

http://codereview.chromium.org/7387010/diff/39053/content/browser/plugin_filt...
File content/browser/plugin_filter.h (right):

http://codereview.chromium.org/7387010/diff/39053/content/browser/plugin_filt...
content/browser/plugin_filter.h:13: class PluginFilter {
On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> nit: i think naming this PluginServiceFilter makes it clear what it's used for

Done.

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

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/02 12:45:08, Bernhard Bauer wrote:
> On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> > On 2011/08/01 18:07:39, Bernhard Bauer wrote:
> > > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> > > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > > > > 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 havinga 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)
> > > > >
> > > > > 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.
> > > >
> > > > You can move the "plugin->path.value() ==
> > > > webkit::npapi::kDefaultPluginLibraryName" check to PluginService.
> > >
> > > 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
> > > :-/
> >
> > 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.
> 
> I found a different way, by removing the default plug-in *before*
> filtering.
> 
> > > > > 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.
> > > >
> > > > 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?
> > >
> > > 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.
> >
> >
> > 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.
> 
> 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),

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.

> so they need
> to jump to the FILE thread anyway. This just makes the thread hopping in
> RenderMessageFilter explicit, and only there.

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.

> 
> > > > > 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).
> > >
> > >

[Bernhard Bauer, 2011-08-02 15:52:46]

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/02 15:13:32, John Abd-El-Malek wrote:
> On 2011/08/02 12:45:08, Bernhard Bauer wrote:
> > On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> > > On 2011/08/01 18:07:39, Bernhard Bauer wrote:
> > > > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> > > > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > > > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > > > > > 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 havinga 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)
> > > > > >
> > > > > > 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.
> > > > >
> > > > > You can move the "plugin->path.value() ==
> > > > > webkit::npapi::kDefaultPluginLibraryName" check to
> > > > > PluginService.
> > > >
> > > > 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 :-/
> > >
> > > 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.
> >
> > I found a different way, by removing the default plug-in *before*
> > filtering.
> >
> > > > > > 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.
> > > > >
> > > > > 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?
> > > >
> > > > 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.
> > >
> > >
> > > 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.
> >
> > 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),
>
> I wouldn't say all clients, RenderMessageFilter uses it on the file
> thread. 

But only because it already jumps to the FILE thread in BrowserMessageFilter ;-)

>  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.
>
> > so they need to jump to the FILE thread anyway. This just makes the
> > thread hopping in RenderMessageFilter explicit, and only there.
>
> 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.

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.

> > > > > > 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-02 16:02:36]

On 2011/08/02 15:52:46, Bernhard Bauer wrote:
>
http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
> File content/browser/content_browser_client.h (right):
> 
>
http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
> content/browser/content_browser_client.h:163: virtual PluginService::Filter*
> CreatePluginFilter(
> On 2011/08/02 15:13:32, John Abd-El-Malek wrote:
> > On 2011/08/02 12:45:08, Bernhard Bauer wrote:
> > > On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> > > > On 2011/08/01 18:07:39, Bernhard Bauer wrote:
> > > > > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> > > > > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > > > > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > > > > > > 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 havinga 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)
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > You can move the "plugin->path.value() ==
> > > > > > webkit::npapi::kDefaultPluginLibraryName" check to
> > > > > > PluginService.
> > > > >
> > > > > 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 :-/
> > > >
> > > > 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.
> > >
> > > I found a different way, by removing the default plug-in *before*
> > > filtering.
> > >
> > > > > > > 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.
> > > > > >
> > > > > > 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?
> > > > >
> > > > > 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.
> > > >
> > > >
> > > > 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.
> > >
> > > 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),
> >
> > I wouldn't say all clients, RenderMessageFilter uses it on the file
> > thread. 
> 
> But only because it already jumps to the FILE thread in BrowserMessageFilter
;-)

right, as I mentioned below. My point was that RMF currently has a one line
switch statement to dispatch this method to the file thread, and this change
adds a bunch of manual work.

> 
> >  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.
> >
> > > so they need to jump to the FILE thread anyway. This just makes the
> > > thread hopping in RenderMessageFilter explicit, and only there.
> >
> > 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.
> 
> 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.

My suggestion was to have a thread-safe mapping that's created on the IO thread,
say each time a new ResourceContext is created.

> 
> Is your objection specifically about the thread jumping in
RenderMessageFilter?

That's only part of it. The other benefits are the ones I outlined in the
beginning, i.e. not increasing ContentBrowserClient API, using one filter for
the lifetime of the browser (which allows us to make a bunch of the
ChromePluginServiceHelper methods private) and avoids always having to go to IO
thread first, keeping thread hopping in RMV automatic.

> 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.
> 
> > > > > > > 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).
> > > > >
> >
> >

[Bernhard Bauer, 2011-08-02 16:13:30]

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
File content/browser/content_browser_client.h (right):

http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
content/browser/content_browser_client.h:163: virtual PluginService::Filter*
CreatePluginFilter(
On 2011/08/02 15:52:47, Bernhard Bauer wrote:
> On 2011/08/02 15:13:32, John Abd-El-Malek wrote:
> > On 2011/08/02 12:45:08, Bernhard Bauer wrote:
> > > On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
> > > > On 2011/08/01 18:07:39, Bernhard Bauer wrote:
> > > > > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
> > > > > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
> > > > > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
> > > > > > > > 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 havinga 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)
> > > > > > >
> > > > > > > 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 ). 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.
> > > > > >
> > > > > > You can move the "plugin->path.value() ==
> > > > > > webkit::npapi::kDefaultPluginLibraryName" check to
> > > > > > PluginService.
> > > > >
> > > > > 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 :-/
> > > >
> > > > 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.
> > >
> > > I found a different way, by removing the default plug-in *before*
> > > filtering.
> > >
> > > > > > > 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.
> > > > > >
> > > > > > 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?
> > > > >
> > > > > 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.
> > > >
> > > >
> > > > 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.
> > >
> > > 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),
> >
> > I wouldn't say all clients, RenderMessageFilter uses it on the file
> > thread.
>
> But only because it already jumps to the FILE thread in
> BrowserMessageFilter ;-)
>
> >  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.
> >
> > > so they need to jump to the FILE thread anyway. This just makes
> > > the thread hopping in RenderMessageFilter explicit, and only
> > > there.
> >
> > 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.
>
> 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.

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?

> > > > > > > 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-02 16:17:12]

On Tue, Aug 2, 2011 at 9:13 AM, <bauerb@chromium.org> wrote:

>
> http://codereview.chromium.**org/7387010/diff/30001/**
>
content/browser/content_**browser_client.h<http://codereview.chromium.org/7387010/diff/30001/content/browser/content_browser_client.h>
> File content/browser/content_**browser_client.h (right):
>
> http://codereview.chromium.**org/7387010/diff/30001/**
>
content/browser/content_**browser_client.h#newcode163<http://codereview.chromium.org/7387010/diff/30001/content/browser/content_browser_client.h#newcode163>
> content/browser/content_**browser_client.h:163: virtual
> PluginService::Filter* CreatePluginFilter(
> On 2011/08/02 15:52:47, Bernhard Bauer wrote:
>
>> On 2011/08/02 15:13:32, John Abd-El-Malek wrote:
>> > On 2011/08/02 12:45:08, Bernhard Bauer wrote:
>> > > On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
>> > > > On 2011/08/01 18:07:39, Bernhard Bauer wrote:
>> > > > > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
>> > > > > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
>> > > > > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
>> > > > > > > > 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 havinga 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)
>> > > > > > >
>> > > > > > > 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 ). 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.
>> > > > > >
>> > > > > > You can move the "plugin->path.value() ==
>> > > > > > webkit::npapi::**kDefaultPluginLibraryName" check to
>> > > > > > PluginService.
>> > > > >
>> > > > > 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 :-/
>> > > >
>> > > > 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.
>> > >
>> > > I found a different way, by removing the default plug-in *before*
>> > > filtering.
>> > >
>> > > > > > > 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.
>> > > > > >
>> > > > > > 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?
>> > > > >
>> > > > > Sorry, that's in
http://codereview.chromium.**org/7477044/<http://codereview.chromium.org/7477...)
I
>> > > > > just wanted to already add all the information we're going to
>> > > > > need later to the interface.
>> > > >
>> > > >
>> > > > 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.
>> > >
>> > > 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),
>> >
>> > I wouldn't say all clients, RenderMessageFilter uses it on the file
>> > thread.
>>
>
>  But only because it already jumps to the FILE thread in
>> BrowserMessageFilter ;-)
>>
>
>  >  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.
>> >
>> > > so they need to jump to the FILE thread anyway. This just makes
>> > > the thread hopping in RenderMessageFilter explicit, and only
>> > > there.
>> >
>> > 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.
>>
>
>  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.
>>
>
> 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?


Not braindead :) But adds a bit of complexity. Asynchronous interfaces,
especially ones that might span threads, add extra refcounting overhead.

Why are you opposed to having a threadsafe ResourceContext-PluginPrefs map?

>
>
>  > > > > > > 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).
>> >
>> >
>> >
>>
>
>
http://codereview.chromium.**org/7387010/<http://codereview.chromium.org/7387...
>

[Bernhard Bauer, 2011-08-02 16:33:14]

On Tue, Aug 2, 2011 at 18:17, John Abd-El-Malek <jam@chromium.org> wrote:
> On Tue, Aug 2, 2011 at 9:13 AM, <bauerb@chromium.org> wrote:
>>
http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
>> File content/browser/content_browser_client.h (right):
>>
>>
>>
http://codereview.chromium.org/7387010/diff/30001/content/browser/content_bro...
>> content/browser/content_browser_client.h:163: virtual
>> PluginService::Filter* CreatePluginFilter(
>> On 2011/08/02 15:52:47, Bernhard Bauer wrote:
>>>
>>> On 2011/08/02 15:13:32, John Abd-El-Malek wrote:
>>> > On 2011/08/02 12:45:08, Bernhard Bauer wrote:
>>> > > On 2011/08/02 04:18:18, John Abd-El-Malek wrote:
>>> > > > On 2011/08/01 18:07:39, Bernhard Bauer wrote:
>>> > > > > On 2011/08/01 17:25:04, John Abd-El-Malek wrote:
>>> > > > > > On 2011/08/01 16:06:12, Bernhard Bauer wrote:
>>> > > > > > > On 2011/08/01 03:25:44, John Abd-El-Malek wrote:
>>> > > > > > > > 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 havinga 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)
>>> > > > > > >
>>> > > > > > > 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 ). 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.
>>> > > > > >
>>> > > > > > You can move the "plugin->path.value() ==
>>> > > > > > webkit::npapi::kDefaultPluginLibraryName" check to
>>> > > > > > PluginService.
>>> > > > >
>>> > > > > 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 :-/
>>> > > >
>>> > > > 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.
>>> > >
>>> > > I found a different way, by removing the default plug-in *before*
>>> > > filtering.
>>> > >
>>> > > > > > > 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.
>>> > > > > >
>>> > > > > > 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?
>>> > > > >
>>> > > > > 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.
>>> > > >
>>> > > >
>>> > > > 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.
>>> > >
>>> > > 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),
>>> >
>>> > I wouldn't say all clients, RenderMessageFilter uses it on the file
>>> > thread.
>>
>>> But only because it already jumps to the FILE thread in
>>> BrowserMessageFilter ;-)
>>
>>> >  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.
>>> >
>>> > > so they need to jump to the FILE thread anyway. This just makes
>>> > > the thread hopping in RenderMessageFilter explicit, and only
>>> > > there.
>>> >
>>> > 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.
>>
>>> 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.
>>
>> 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?
>
> Not braindead :) But adds a bit of complexity. Asynchronous interfaces,
> especially ones that might span threads, add extra refcounting overhead.

Do I need refcounting? I could just use NewRunnableMethod on
long-lived or already refcounted objects and pass ownership of the
callback on until the last hop.

> Why are you opposed to having a threadsafe ResourceContext-PluginPrefs map?

It would add complexity as well, and in a location that hasn't even by
itself a lot to do with plugins :-)

>>> > > > > > > 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).
>>> >
>>> >
>>> >
>>
>> http://codereview.chromium.org/7387010/
>
>

[Bernhard Bauer, 2011-08-03 14:47:32]

PTAL.

http://codereview.chromium.org/7387010/diff/15032/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/7387010/diff/15032/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:394:
content::BrowserContext* browser_context = NULL;  // XXX
This is the only problem; I don't have a BrowserContext here, just a
ResourceContext.

[jam, 2011-08-03 16:20:43]

http://codereview.chromium.org/7387010/diff/15032/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/7387010/diff/15032/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:394:
content::BrowserContext* browser_context = NULL;  // XXX
On 2011/08/03 14:47:33, Bernhard Bauer wrote:
> This is the only problem; I don't have a BrowserContext here, just a
> ResourceContext.

Why not use ResourceContext instead of BrowserContext for the filter? It seems
easier to get, since more threads have access to the RC than the BC (i.e. UI
thread has access to both, but most on IO thread and the message filters only
have RC)

[Bernhard Bauer, 2011-08-03 16:45:36]

http://codereview.chromium.org/7387010/diff/15032/content/browser/renderer_ho...
File content/browser/renderer_host/buffered_resource_handler.cc (right):

http://codereview.chromium.org/7387010/diff/15032/content/browser/renderer_ho...
content/browser/renderer_host/buffered_resource_handler.cc:394:
content::BrowserContext* browser_context = NULL;  // XXX
On 2011/08/03 16:20:43, John Abd-El-Malek wrote:
> On 2011/08/03 14:47:33, Bernhard Bauer wrote:
> > This is the only problem; I don't have a BrowserContext here, just a
> > ResourceContext.
> 
> Why not use ResourceContext instead of BrowserContext for the filter? It seems
> easier to get, since more threads have access to the RC than the BC (i.e. UI
> thread has access to both, but most on IO thread and the message filters only
> have RC)

Because a BrowserContext is a Profile, and we already have a mechanism for tying
objects to a Profile without the Profile having to know about it (see
http://crbug.com/77155). We don't have that for ResourceContext.

I guess I could update the map in the constructor/destructor for ProfileIOData
though...

[Bernhard Bauer, 2011-08-09 13:01:23]

PTAL? I rebased this CL on top of http://codereview.chromium.org/7564006/, which
adds PluginPrefs.

Note that there's still a slight race condition here at shutdown: the
ResourceContext could be destroyed on the IO thread while we're on the FILE
thread, so we wouldn't find the corresponding PluginPrefs anymore.

[jam, 2011-08-10 06:31:51]

lgtm

re the race condition, it seems harmless since the renderers should already be
shut down?

http://codereview.chromium.org/7387010/diff/53001/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_filter.h (right):

http://codereview.chromium.org/7387010/diff/53001/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_filter.h:79: mutable base::Lock lock_;  //
Guards access to member variables.
nit: doesn't look like this needs to be mutable

http://codereview.chromium.org/7387010/diff/53001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7387010/diff/53001/content/browser/plugin_serv...
content/browser/plugin_service.h:181: // Weak pointer; singleton.
nit: no need to for the content layer to know that the embedder's object is a
singelton. i.e. it can just say it's guaranteed to outlive this object

[Bernhard Bauer, 2011-08-10 09:18:34]

On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> re the race condition, it seems harmless since the renderers should already be
> shut down?

I'm not sure; that's why I tried to be on the safe side ;-) But if it happens,
it'll manifest as a NULL dereference, so we'll get crash reports pointing to it.

http://codereview.chromium.org/7387010/diff/53001/chrome/browser/chrome_plugi...
File chrome/browser/chrome_plugin_service_filter.h (right):

http://codereview.chromium.org/7387010/diff/53001/chrome/browser/chrome_plugi...
chrome/browser/chrome_plugin_service_filter.h:79: mutable base::Lock lock_;  //
Guards access to member variables.
On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> nit: doesn't look like this needs to be mutable

I used it in ShouldUsePlugin which was const, but I made it non-const now.

http://codereview.chromium.org/7387010/diff/53001/content/browser/plugin_serv...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7387010/diff/53001/content/browser/plugin_serv...
content/browser/plugin_service.h:181: // Weak pointer; singleton.
On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> nit: no need to for the content layer to know that the embedder's object is a
> singelton. i.e. it can just say it's guaranteed to outlive this object

Done.

[Bernhard Bauer, 2011-08-19 13:18:36]

On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> lgtm
> 
> re the race condition, it seems harmless since the renderers should already be
> shut down?

So, looks like the race condition *is* happening after all:

http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/st...
(ExtensionCrxInstallerTest.Whitelisting)

I've debugged it locally, and we're indeed destroying the profile while loading
the plugins on the file thread :-/

[Bernhard Bauer, 2011-08-30 13:39:38]

On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> > lgtm
> > 
> > re the race condition, it seems harmless since the renderers should already
be
> > shut down?
> 
> So, looks like the race condition *is* happening after all:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/st...
> (ExtensionCrxInstallerTest.Whitelisting)
> 
> I've debugged it locally, and we're indeed destroying the profile while
loading
> the plugins on the file thread :-/

I changed the code to handle that case gracefully. Do you want to take another
look?

[jam, 2011-08-30 16:55:07]

On 2011/08/30 13:39:38, Bernhard Bauer wrote:
> On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> > > lgtm
> > > 
> > > re the race condition, it seems harmless since the renderers should
already
> be
> > > shut down?
> > 
> > So, looks like the race condition *is* happening after all:
> > 
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/st...
> > (ExtensionCrxInstallerTest.Whitelisting)
> > 
> > I've debugged it locally, and we're indeed destroying the profile while
> loading
> > the plugins on the file thread :-/
> 
> I changed the code to handle that case gracefully. Do you want to take another
> look?

this change is pretty large, so can you tell me what changed so I can just look
at that? i.e. either patchset x to y?

[Bernhard Bauer, 2011-08-30 17:09:34]

On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> > > > lgtm
> > > > 
> > > > re the race condition, it seems harmless since the renderers should
> already
> > be
> > > > shut down?
> > > 
> > > So, looks like the race condition *is* happening after all:
> > > 
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/st...
> > > (ExtensionCrxInstallerTest.Whitelisting)
> > > 
> > > I've debugged it locally, and we're indeed destroying the profile while
> > loading
> > > the plugins on the file thread :-/
> > 
> > I changed the code to handle that case gracefully. Do you want to take
another
> > look?
> 
> this change is pretty large, so can you tell me what changed so I can just
look
> at that? i.e. either patchset x to y?

The only real change is in chrome_plugin_service_filter.cc, when we don't find
the PluginPrefs.

[jam, 2011-08-30 17:13:06]

On Tue, Aug 30, 2011 at 10:09 AM, <bauerb@chromium.org> wrote:

> On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
>
>> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
>> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
>> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
>> > > > lgtm
>> > > >
>> > > > re the race condition, it seems harmless since the renderers should
>> already
>> > be
>> > > > shut down?
>> > >
>> > > So, looks like the race condition *is* happening after all:
>> > >
>> > >
>> >
>>
>
> http://build.chromium.org/p/**tryserver.chromium/builders/**
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
>
>> > > (ExtensionCrxInstallerTest.**Whitelisting)
>> > >
>> > > I've debugged it locally, and we're indeed destroying the profile
>> while
>> > loading
>> > > the plugins on the file thread :-/
>> >
>> > I changed the code to handle that case gracefully. Do you want to take
>>
> another
>
>> > look?
>>
>
>  this change is pretty large, so can you tell me what changed so I can just
>>
> look
>
>> at that? i.e. either patchset x to y?
>>
>
> The only real change is in chrome_plugin_service_filter.**cc, when we
> don't find
> the PluginPrefs.


patchsets to diff?


>
>
>
http://codereview.chromium.**org/7387010/<http://codereview.chromium.org/7387...
>

[Bernhard Bauer, 2011-08-30 20:49:11]

On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
> On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org> wrote:
> 
> > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
> >
> >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
> >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> >> > > > lgtm
> >> > > >
> >> > > > re the race condition, it seems harmless since the renderers should
> >> already
> >> > be
> >> > > > shut down?
> >> > >
> >> > > So, looks like the race condition *is* happening after all:
> >> > >
> >> > >
> >> >
> >>
> >
> > http://build.chromium.org/p/**tryserver.chromium/builders/**
> >
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
> >
> >> > > (ExtensionCrxInstallerTest.**Whitelisting)
> >> > >
> >> > > I've debugged it locally, and we're indeed destroying the profile
> >> while
> >> > loading
> >> > > the plugins on the file thread :-/
> >> >
> >> > I changed the code to handle that case gracefully. Do you want to take
> >>
> > another
> >
> >> > look?
> >>
> >
> >  this change is pretty large, so can you tell me what changed so I can just
> >>
> > look
> >
> >> at that? i.e. either patchset x to y?
> >>
> >
> > The only real change is in chrome_plugin_service_filter.**cc, when we
> > don't find
> > the PluginPrefs.
> 
> 
> patchsets to diff?

http://codereview.chromium.org/7387010/diff2/53001:72001/chrome/browser/chrom...

The other changes between those patch sets should only be from syncing.

> >
> >
> >
>
http://codereview.chromium.**org/7387010/%3Chttp://codereview.chromium.org/73...>
> >

[jam, 2011-08-30 21:17:17]

On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
> On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org> wrote:
> 
> > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
> >
> >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
> >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> >> > > > lgtm
> >> > > >
> >> > > > re the race condition, it seems harmless since the renderers should
> >> already
> >> > be
> >> > > > shut down?
> >> > >
> >> > > So, looks like the race condition *is* happening after all:
> >> > >
> >> > >
> >> >
> >>
> >
> > http://build.chromium.org/p/**tryserver.chromium/builders/**
> >
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
> >
> >> > > (ExtensionCrxInstallerTest.**Whitelisting)
> >> > >
> >> > > I've debugged it locally, and we're indeed destroying the profile
> >> while
> >> > loading
> >> > > the plugins on the file thread :-/
> >> >
> >> > I changed the code to handle that case gracefully. Do you want to take
> >>
> > another
> >
> >> > look?
> >>
> >
> >  this change is pretty large, so can you tell me what changed so I can just
> >>
> > look
> >
> >> at that? i.e. either patchset x to y?
> >>
> >
> > The only real change is in chrome_plugin_service_filter.**cc, when we
> > don't find
> > the PluginPrefs.
> 
> 
> patchsets to diff?
> 
> 
> >
> >
> >
>
http://codereview.chromium.**org/7387010/%3Chttp://codereview.chromium.org/73...>
> >

cool, still lgtm. do we want that warning if it can happen at shutdown?

[Bernhard Bauer, 2011-08-30 21:32:38]

On 2011/08/30 21:17:17, John Abd-El-Malek wrote:
> On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
> > On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org> wrote:
> > 
> > > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
> > >
> > >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
> > >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> > >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> > >> > > > lgtm
> > >> > > >
> > >> > > > re the race condition, it seems harmless since the renderers should
> > >> already
> > >> > be
> > >> > > > shut down?
> > >> > >
> > >> > > So, looks like the race condition *is* happening after all:
> > >> > >
> > >> > >
> > >> >
> > >>
> > >
> > > http://build.chromium.org/p/**tryserver.chromium/builders/**
> > >
> >
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
> > >
> > >> > > (ExtensionCrxInstallerTest.**Whitelisting)
> > >> > >
> > >> > > I've debugged it locally, and we're indeed destroying the profile
> > >> while
> > >> > loading
> > >> > > the plugins on the file thread :-/
> > >> >
> > >> > I changed the code to handle that case gracefully. Do you want to take
> > >>
> > > another
> > >
> > >> > look?
> > >>
> > >
> > >  this change is pretty large, so can you tell me what changed so I can
just
> > >>
> > > look
> > >
> > >> at that? i.e. either patchset x to y?
> > >>
> > >
> > > The only real change is in chrome_plugin_service_filter.**cc, when we
> > > don't find
> > > the PluginPrefs.
> > 
> > 
> > patchsets to diff?
> > 
> > 
> > >
> > >
> > >
> >
>
http://codereview.chromium.**org/7387010/%253Chttp://codereview.chromium.org/...>
> > >
> 
> cool, still lgtm. do we want that warning if it can happen at shutdown?

Hm, I wanted to have the warning in case this happens during the browser run (or
not late enough in the shutdown process for the request to be swallowed), so it
can be debugged a bit more easily.

I think that this will happen more likely in tests anyway, as we have very short
browser runtimes there.

[jam, 2011-08-30 21:46:59]

On Tue, Aug 30, 2011 at 2:32 PM, <bauerb@chromium.org> wrote:

> On 2011/08/30 21:17:17, John Abd-El-Malek wrote:
>
>> On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
>> > On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org> wrote:
>> >
>> > > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
>> > >
>> > >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
>> > >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
>> > >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
>> > >> > > > lgtm
>> > >> > > >
>> > >> > > > re the race condition, it seems harmless since the renderers
>> should
>> > >> already
>> > >> > be
>> > >> > > > shut down?
>> > >> > >
>> > >> > > So, looks like the race condition *is* happening after all:
>> > >> > >
>> > >> > >
>> > >> >
>> > >>
>> > >
>> > >
http://build.chromium.org/p/****tryserver.chromium/builders/**<http://build.c...
>> > >
>> >
>>
>
> linux/builds/43645/steps/****browser_tests/logs<http://**
> build.chromium.org/p/**tryserver.chromium/builders/**
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
> >
>
>> > >
>> > >> > > (ExtensionCrxInstallerTest.****Whitelisting)
>> > >> > >
>> > >> > > I've debugged it locally, and we're indeed destroying the profile
>> > >> while
>> > >> > loading
>> > >> > > the plugins on the file thread :-/
>> > >> >
>> > >> > I changed the code to handle that case gracefully. Do you want to
>> take
>> > >>
>> > > another
>> > >
>> > >> > look?
>> > >>
>> > >
>> > >  this change is pretty large, so can you tell me what changed so I can
>>
> just
>
>> > >>
>> > > look
>> > >
>> > >> at that? i.e. either patchset x to y?
>> > >>
>> > >
>> > > The only real change is in chrome_plugin_service_filter.****cc, when
>> we
>> > > don't find
>> > > the PluginPrefs.
>> >
>> >
>> > patchsets to diff?
>> >
>> >
>> > >
>> > >
>> > >
>> >
>>
>
> http://codereview.chromium.****org/7387010/%253Chttp://codere**
> view.chromium.org/7387010/ <http://codereview.chromium.org/7387010/>>
>
>  > >
>>
>
>  cool, still lgtm. do we want that warning if it can happen at shutdown?
>>
>
> Hm, I wanted to have the warning in case this happens during the browser
> run (or
> not late enough in the shutdown process for the request to be swallowed),
> so it
> can be debugged a bit more easily.
>
> I think that this will happen more likely in tests anyway, as we have very
> short
> browser runtimes there.


If this can happen legitimately, then I don't think we should add a warning.
This will make developers looking at the output think that something wrong
is happening, and someone can waste time looking into it. We should only be
adding warnings if something wrong happens.


>
>
>
http://codereview.chromium.**org/7387010/<http://codereview.chromium.org/7387...
>

[Bernhard Bauer, 2011-08-30 22:12:20]

On 2011/08/30 21:46:59, John Abd-El-Malek wrote:
> On Tue, Aug 30, 2011 at 2:32 PM, <mailto:bauerb@chromium.org> wrote:
> 
> > On 2011/08/30 21:17:17, John Abd-El-Malek wrote:
> >
> >> On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
> >> > On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org> wrote:
> >> >
> >> > > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
> >> > >
> >> > >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
> >> > >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
> >> > >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
> >> > >> > > > lgtm
> >> > >> > > >
> >> > >> > > > re the race condition, it seems harmless since the renderers
> >> should
> >> > >> already
> >> > >> > be
> >> > >> > > > shut down?
> >> > >> > >
> >> > >> > > So, looks like the race condition *is* happening after all:
> >> > >> > >
> >> > >> > >
> >> > >> >
> >> > >>
> >> > >
> >> > >
>
http://build.chromium.org/p/****tryserver.chromium/builders/**%3Chttp://build...>
> >> > >
> >> >
> >>
> >
> > linux/builds/43645/steps/****browser_tests/logs<http://**
> > build.chromium.org/p/**tryserver.chromium/builders/**
> >
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
> > >
> >
> >> > >
> >> > >> > > (ExtensionCrxInstallerTest.****Whitelisting)
> >> > >> > >
> >> > >> > > I've debugged it locally, and we're indeed destroying the profile
> >> > >> while
> >> > >> > loading
> >> > >> > > the plugins on the file thread :-/
> >> > >> >
> >> > >> > I changed the code to handle that case gracefully. Do you want to
> >> take
> >> > >>
> >> > > another
> >> > >
> >> > >> > look?
> >> > >>
> >> > >
> >> > >  this change is pretty large, so can you tell me what changed so I can
> >>
> > just
> >
> >> > >>
> >> > > look
> >> > >
> >> > >> at that? i.e. either patchset x to y?
> >> > >>
> >> > >
> >> > > The only real change is in chrome_plugin_service_filter.****cc, when
> >> we
> >> > > don't find
> >> > > the PluginPrefs.
> >> >
> >> >
> >> > patchsets to diff?
> >> >
> >> >
> >> > >
> >> > >
> >> > >
> >> >
> >>
> >
> > http://codereview.chromium.****org/7387010/%25253Chttp://codere**
> > view.chromium.org/7387010/ <http://codereview.chromium.org/7387010/>>
> >
> >  > >
> >>
> >
> >  cool, still lgtm. do we want that warning if it can happen at shutdown?
> >>
> >
> > Hm, I wanted to have the warning in case this happens during the browser
> > run (or
> > not late enough in the shutdown process for the request to be swallowed),
> > so it
> > can be debugged a bit more easily.
> >
> > I think that this will happen more likely in tests anyway, as we have very
> > short
> > browser runtimes there.
> 
> 
> If this can happen legitimately, then I don't think we should add a warning.
> This will make developers looking at the output think that something wrong
> is happening, and someone can waste time looking into it. We should only be
> adding warnings if something wrong happens.

But something wrong *is* happening, it's just that we don't care when we're
shutting down anyway ;-)

I could add an explanation that this is harmless if it happens at shutdown, but
a problem otherwise?

> >
> >
> >
>
http://codereview.chromium.**org/7387010/%3Chttp://codereview.chromium.org/73...>
> >

[jam, 2011-08-30 22:18:13]

On Tue, Aug 30, 2011 at 3:12 PM, <bauerb@chromium.org> wrote:

> On 2011/08/30 21:46:59, John Abd-El-Malek wrote:
>
>  On Tue, Aug 30, 2011 at 2:32 PM, <mailto:bauerb@chromium.org> wrote:
>>
>
>  > On 2011/08/30 21:17:17, John Abd-El-Malek wrote:
>> >
>> >> On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
>> >> > On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org>
>> wrote:
>> >> >
>> >> > > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
>> >> > >
>> >> > >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
>> >> > >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
>> >> > >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
>> >> > >> > > > lgtm
>> >> > >> > > >
>> >> > >> > > > re the race condition, it seems harmless since the renderers
>> >> should
>> >> > >> already
>> >> > >> > be
>> >> > >> > > > shut down?
>> >> > >> > >
>> >> > >> > > So, looks like the race condition *is* happening after all:
>> >> > >> > >
>> >> > >> > >
>> >> > >> >
>> >> > >>
>> >> > >
>> >> > >
>>
>
> http://build.chromium.org/p/******tryserver.chromium/builders/**
>
**%3Chttp://build.chromium.**org/p/**tryserver.chromium/**builders/**<http://build.chromium.org/p/****tryserver.chromium/builders/**%3Chttp://build.chromium.org/p/**tryserver.chromium/builders/**>
> >
>
>  >> > >
>> >> >
>> >>
>> >
>> > linux/builds/43645/steps/******browser_tests/logs<http://**
>> >
build.chromium.org/p/****tryserver.chromium/builders/**<http://build.chromium.org/p/**tryserver.chromium/builders/**>
>> >
>>
>
> linux/builds/43645/steps/****browser_tests/logs<http://**
> build.chromium.org/p/**tryserver.chromium/builders/**
>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
> >
>
>> > >
>> >
>> >> > >
>> >> > >> > > (ExtensionCrxInstallerTest.******Whitelisting)
>> >> > >> > >
>> >> > >> > > I've debugged it locally, and we're indeed destroying the
>> profile
>> >> > >> while
>> >> > >> > loading
>> >> > >> > > the plugins on the file thread :-/
>> >> > >> >
>> >> > >> > I changed the code to handle that case gracefully. Do you want
>> to
>> >> take
>> >> > >>
>> >> > > another
>> >> > >
>> >> > >> > look?
>> >> > >>
>> >> > >
>> >> > >  this change is pretty large, so can you tell me what changed so I
>> can
>> >>
>> > just
>> >
>> >> > >>
>> >> > > look
>> >> > >
>> >> > >> at that? i.e. either patchset x to y?
>> >> > >>
>> >> > >
>> >> > > The only real change is in chrome_plugin_service_filter.******cc,
>> when
>> >> we
>> >> > > don't find
>> >> > > the PluginPrefs.
>> >> >
>> >> >
>> >> > patchsets to diff?
>> >> >
>> >> >
>> >> > >
>> >> > >
>> >> > >
>> >> >
>> >>
>> >
>> > http://codereview.chromium.******org/7387010/%25253Chttp://**codere**
>> > view.chromium.org/7387010/
<http://codereview.chromium.**org/7387010/<http://codereview.chromium.org/7387...
>> >>
>>
>> >
>> >  > >
>> >>
>> >
>> >  cool, still lgtm. do we want that warning if it can happen at shutdown?
>> >>
>> >
>> > Hm, I wanted to have the warning in case this happens during the browser
>> > run (or
>> > not late enough in the shutdown process for the request to be
>> swallowed),
>> > so it
>> > can be debugged a bit more easily.
>> >
>> > I think that this will happen more likely in tests anyway, as we have
>> very
>> > short
>> > browser runtimes there.
>>
>
>
>  If this can happen legitimately, then I don't think we should add a
>> warning.
>> This will make developers looking at the output think that something wrong
>> is happening, and someone can waste time looking into it. We should only
>> be
>> adding warnings if something wrong happens.
>>
>
> But something wrong *is* happening, it's just that we don't care when we're
> shutting down anyway ;-)
>
> I could add an explanation that this is harmless if it happens at shutdown,
> but
> a problem otherwise?


would it be possible to check a global or something to detect that shutdown
is happening, and only logging if it's not?

>
>
>  >
>> >
>> >
>>
>
> http://codereview.chromium.****org/7387010/%3Chttp://coderevi**
> ew.chromium.org/7387010/ <http://codereview.chromium.org/7387010/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/7387010/<http://codereview.chromium.org/7387...
>

[Bernhard Bauer, 2011-08-31 12:07:41]

On Wed, Aug 31, 2011 at 00:18, John Abd-El-Malek <jam@chromium.org> wrote:
>
>
> On Tue, Aug 30, 2011 at 3:12 PM, <bauerb@chromium.org> wrote:
>>
>> On 2011/08/30 21:46:59, John Abd-El-Malek wrote:
>>>
>>> On Tue, Aug 30, 2011 at 2:32 PM, <mailto:bauerb@chromium.org> wrote:
>>
>>> > On 2011/08/30 21:17:17, John Abd-El-Malek wrote:
>>> >
>>> >> On 2011/08/30 17:13:06, John Abd-El-Malek wrote:
>>> >> > On Tue, Aug 30, 2011 at 10:09 AM, <mailto:bauerb@chromium.org>
>>> >> > wrote:
>>> >> >
>>> >> > > On 2011/08/30 16:55:07, John Abd-El-Malek wrote:
>>> >> > >
>>> >> > >> On 2011/08/30 13:39:38, Bernhard Bauer wrote:
>>> >> > >> > On 2011/08/19 13:18:36, Bernhard Bauer wrote:
>>> >> > >> > > On 2011/08/10 06:31:51, John Abd-El-Malek wrote:
>>> >> > >> > > > lgtm
>>> >> > >> > > >
>>> >> > >> > > > re the race condition, it seems harmless since the
>>> >> > >> > > > renderers
>>> >> should
>>> >> > >> already
>>> >> > >> > be
>>> >> > >> > > > shut down?
>>> >> > >> > >
>>> >> > >> > > So, looks like the race condition *is* happening after all:
>>> >> > >> > >
>>> >> > >> > >
>>> >> > >> >
>>> >> > >>
>>> >> > >
>>> >> > >
>>
>>
>>
http://build.chromium.org/p/****tryserver.chromium/builders/**%3Chttp://build...>
>>>
>>> >> > >
>>> >> >
>>> >>
>>> >
>>> > linux/builds/43645/steps/****browser_tests/logs<http://**
>>> > build.chromium.org/p/**tryserver.chromium/builders/**
>>> >
>>
>>
>>
linux/builds/43645/steps/**browser_tests/logs<http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/43645/steps/browser_tests/logs>
>>>
>>> > >
>>> >
>>> >> > >
>>> >> > >> > > (ExtensionCrxInstallerTest.****Whitelisting)
>>> >> > >> > >
>>> >> > >> > > I've debugged it locally, and we're indeed destroying the
>>> >> > >> > > profile
>>> >> > >> while
>>> >> > >> > loading
>>> >> > >> > > the plugins on the file thread :-/
>>> >> > >> >
>>> >> > >> > I changed the code to handle that case gracefully. Do you want
>>> >> > >> > to
>>> >> take
>>> >> > >>
>>> >> > > another
>>> >> > >
>>> >> > >> > look?
>>> >> > >>
>>> >> > >
>>> >> > >  this change is pretty large, so can you tell me what changed so I
>>> >> > > can
>>> >>
>>> > just
>>> >
>>> >> > >>
>>> >> > > look
>>> >> > >
>>> >> > >> at that? i.e. either patchset x to y?
>>> >> > >>
>>> >> > >
>>> >> > > The only real change is in chrome_plugin_service_filter.****cc,
>>> >> > > when
>>> >> we
>>> >> > > don't find
>>> >> > > the PluginPrefs.
>>> >> >
>>> >> >
>>> >> > patchsets to diff?
>>> >> >
>>> >> >
>>> >> > >
>>> >> > >
>>> >> > >
>>> >> >
>>> >>
>>> >
>>> > http://codereview.chromium.****org/7387010/%25253Chttp://codere**
>>> > view.chromium.org/7387010/ <http://codereview.chromium.org/7387010/>>
>>> >
>>> >  > >
>>> >>
>>> >
>>> >  cool, still lgtm. do we want that warning if it can happen at
>>> > shutdown?
>>> >>
>>> >
>>> > Hm, I wanted to have the warning in case this happens during the
>>> > browser
>>> > run (or
>>> > not late enough in the shutdown process for the request to be
>>> > swallowed),
>>> > so it
>>> > can be debugged a bit more easily.
>>> >
>>> > I think that this will happen more likely in tests anyway, as we have
>>> > very
>>> > short
>>> > browser runtimes there.
>>
>>
>>> If this can happen legitimately, then I don't think we should add a
>>> warning.
>>> This will make developers looking at the output think that something
>>> wrong
>>> is happening, and someone can waste time looking into it. We should only
>>> be
>>> adding warnings if something wrong happens.
>>
>> But something wrong *is* happening, it's just that we don't care when
>> we're
>> shutting down anyway ;-)
>>
>> I could add an explanation that this is harmless if it happens at
>> shutdown, but
>> a problem otherwise?
>
> would it be possible to check a global or something to detect that shutdown
> is happening, and only logging if it's not?

Actually, the ResourceContext is destroyed on *profile* shutdown, so
it might happen in the middle of the browser run. I've removed the
warning.

>>> >
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.**org/7387010/%3Chttp://codereview.chromium.org/73...>
>>>
>>> >
>>
>>
>>
>> http://codereview.chromium.org/7387010/
>
>

[Bernhard Bauer, 2011-08-31 21:56:53]

Sorry to bother you again, another small change to fix a unit test crash: patch
set 36 to 37.

[willchan no longer on Chromium, 2011-09-01 19:50:24]

On 2011/08/31 21:56:53, Bernhard Bauer wrote:
> Sorry to bother you again, another small change to fix a unit test crash:
patch
> set 36 to 37.

Thanks for the context info you sent me offline. I've looked at this now. As I
said earlier, I hate globals. This code already used globals originally, so I
don't think this is any worse than the previous.

That said, I'd prefer to minimize the spread of the globals. We already do much
of this for Profile. Can we keep profile_io_data clean and instead move this
code to Profile? It constructs the ProfileIOData and calls ShutdownOnUIThread(),
so we should be able to update the map there instead of in profile_io_data.

Also, if the ResourceContext* isn't to be used, and is only used as an id,
perhaps we should pass it as a void* instead?

As a side note, I would have expected us to use message passing instead of
locking here. It seems like the map could live on the FILE thread and have the
updates from the UI thread forwarded to the FILE thread.

[Bernhard Bauer, 2011-09-01 22:47:28]

On 2011/09/01 19:50:24, willchan wrote:
> On 2011/08/31 21:56:53, Bernhard Bauer wrote:
> > Sorry to bother you again, another small change to fix a unit test crash:
> patch
> > set 36 to 37.
> 
> Thanks for the context info you sent me offline. I've looked at this now. As I
> said earlier, I hate globals. This code already used globals originally, so I
> don't think this is any worse than the previous.
> 
> That said, I'd prefer to minimize the spread of the globals. We already do
much
> of this for Profile. Can we keep profile_io_data clean and instead move this
> code to Profile? It constructs the ProfileIOData and calls
ShutdownOnUIThread(),
> so we should be able to update the map there instead of in profile_io_data.

Sure, if that makes it clearer.

> Also, if the ResourceContext* isn't to be used, and is only used as an id,
> perhaps we should pass it as a void* instead?

Done.

> As a side note, I would have expected us to use message passing instead of
> locking here. It seems like the map could live on the FILE thread and have the
> updates from the UI thread forwarded to the FILE thread.

The thing is, sometimes we call GetPluginInfo from other threads as well, when
we're fine with possibly using a stale plugin list, so we need to be prepared to
be called from any thread :-/

[willchan no longer on Chromium, 2011-09-01 23:34:44]

profiles/ LGTM
It's a bit annoying that the OffTheRecordProfile and ProfileImpl don't compose a
common base implementation class. Oh well.

http://codereview.chromium.org/7387010/diff/93050/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7387010/diff/93050/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:18: #include
"chrome/browser/chrome_plugin_service_filter.h"
Remove

[willchan no longer on Chromium, 2011-09-01 23:36:18]

On 2011/09/01 22:47:28, Bernhard Bauer wrote:
> On 2011/09/01 19:50:24, willchan wrote:
> > On 2011/08/31 21:56:53, Bernhard Bauer wrote:
> > > Sorry to bother you again, another small change to fix a unit test crash:
> > patch
> > > set 36 to 37.
> > 
> > Thanks for the context info you sent me offline. I've looked at this now. As
I
> > said earlier, I hate globals. This code already used globals originally, so
I
> > don't think this is any worse than the previous.
> > 
> > That said, I'd prefer to minimize the spread of the globals. We already do
> much
> > of this for Profile. Can we keep profile_io_data clean and instead move this
> > code to Profile? It constructs the ProfileIOData and calls
> ShutdownOnUIThread(),
> > so we should be able to update the map there instead of in profile_io_data.
> 
> Sure, if that makes it clearer.
> 
> > Also, if the ResourceContext* isn't to be used, and is only used as an id,
> > perhaps we should pass it as a void* instead?
> 
> Done.
> 
> > As a side note, I would have expected us to use message passing instead of
> > locking here. It seems like the map could live on the FILE thread and have
the
> > updates from the UI thread forwarded to the FILE thread.
> 
> The thing is, sometimes we call GetPluginInfo from other threads as well, when
> we're fine with possibly using a stale plugin list, so we need to be prepared
to
> be called from any thread :-/

Does GetPluginInfo need to be handled synchronously? Otherwise, the solution we
adopt elsewhere in code is to service is asynchronously and proxy the lookup to
the owning thread.

[Bernhard Bauer, 2011-09-02 08:41:47]

On 2011/09/01 23:36:18, willchan wrote:
> Does GetPluginInfo need to be handled synchronously? Otherwise, the solution
we
> adopt elsewhere in code is to service is asynchronously and proxy the lookup
to
> the owning thread.

Usually we do that (at the call sites of GetPluginInfo), but for example in one
case we're in a Cocoa callback on the UI thread, so we have do it synchronously.

http://codereview.chromium.org/7387010/diff/93050/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/7387010/diff/93050/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:18: #include
"chrome/browser/chrome_plugin_service_filter.h"
On 2011/09/01 23:34:44, willchan wrote:
> Remove

Done.