Refactor PluginService to take PluginList as a dependency.

Refactor PluginService to take PluginList as a dependency.

Also creates MockPluginService for use in tests.

BUG=103788, chromium-os:22447
TEST=none

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

[Bernhard Bauer, 2011-11-07 22:37:50]

http://codereview.chromium.org/8493019/diff/1/content/browser/plugin_service.h
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/8493019/diff/1/content/browser/plugin_service....
content/browser/plugin_service.h:185: PluginService(webkit::npapi::PluginList*
plugin_list = NULL);
Could you have a protected mutator to set the plugin list to get around the
default parameter here? We could probably move some of the initialization code
in the constructor that uses |plugin_list| to the client.

http://codereview.chromium.org/8493019/diff/1/content/test/mock_plugin_service.h
File content/test/mock_plugin_service.h (right):

http://codereview.chromium.org/8493019/diff/1/content/test/mock_plugin_servic...
content/test/mock_plugin_service.h:26: class MockPluginService : public
PluginService {
Could we call this TestingPluginService? If I read "mock", I assume it's
something I can set expectations on.

http://codereview.chromium.org/8493019/diff/1/content/test/mock_plugin_servic...
content/test/mock_plugin_service.h:28:
MockPluginService(webkit::npapi::PluginList* plugin_list);
Could you demand a MockPluginList here, or are there cases where we'd want to
pass in a real PluginList?

[Robert Sesek, 2011-11-07 23:05:48]

http://codereview.chromium.org/8493019/diff/1/content/browser/plugin_service.h
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/8493019/diff/1/content/browser/plugin_service....
content/browser/plugin_service.h:185: PluginService(webkit::npapi::PluginList*
plugin_list = NULL);
On 2011/11/07 22:37:50, Bernhard Bauer wrote:
> Could you have a protected mutator to set the plugin list to get around the
> default parameter here? We could probably move some of the initialization code
> in the constructor that uses |plugin_list| to the client.

No, but I can create a custom singleton traits.

http://codereview.chromium.org/8493019/diff/1/content/test/mock_plugin_service.h
File content/test/mock_plugin_service.h (right):

http://codereview.chromium.org/8493019/diff/1/content/test/mock_plugin_servic...
content/test/mock_plugin_service.h:26: class MockPluginService : public
PluginService {
On 2011/11/07 22:37:50, Bernhard Bauer wrote:
> Could we call this TestingPluginService? If I read "mock", I assume it's
> something I can set expectations on.

Sure. The other "mocks" are also not true GMock mocks, so I went with that.

http://codereview.chromium.org/8493019/diff/1/content/test/mock_plugin_servic...
content/test/mock_plugin_service.h:28:
MockPluginService(webkit::npapi::PluginList* plugin_list);
On 2011/11/07 22:37:50, Bernhard Bauer wrote:
> Could you demand a MockPluginList here, or are there cases where we'd want to
> pass in a real PluginList?

I thought about it, but there's no reason to be that specific yet.

[jam, 2011-11-07 23:18:35]

I dislike complicating our already complicated production code for the sake of
tests. Can't we just have a ForTesting method that disables that CHECK?

[Robert Sesek, 2011-11-07 23:20:50]

On 2011/11/07 23:18:35, John Abd-El-Malek wrote:
> I dislike complicating our already complicated production code for the sake of
> tests. Can't we just have a ForTesting method that disables that CHECK?

No, because I discovered that I haven't updated PluginPrefs to use
PluginService, and that test uses MockPluginList and shouldn't use the real
plugin loader. It also is slower in tests to use the OOP/real plugin loader when
they could instead use this one.

[Bernhard Bauer, 2011-11-07 23:22:01]

On 2011/11/07 23:18:35, John Abd-El-Malek wrote:
> I dislike complicating our already complicated production code for the sake of
> tests. Can't we just have a ForTesting method that disables that CHECK?

I think it's a Good Thing(tm) if we're independent from the set of locally
installed plug-ins, both to avoid unnecessary disk accesses, and to be able to
test plug-in settings without actually needing to have the plug-in installed.

[Dirk Pranke, 2011-11-08 04:44:05]

FYI, I landed http://codereview.chromium.org/8493026/ which also changes
PluginService and has some PluginList-related things, so you'll have some
merging to do (hopefully not too much).

[Robert Sesek, 2011-11-08 16:05:46]

On 2011/11/08 04:44:05, Dirk Pranke wrote:
> FYI, I landed http://codereview.chromium.org/8493026/ which also changes
> PluginService and has some PluginList-related things, so you'll have some
> merging to do (hopefully not too much).

Thanks for the note.

All rebased and ready for another look.

[Bernhard Bauer, 2011-11-08 17:03:22]

http://codereview.chromium.org/8493019/diff/7002/content/browser/plugin_servi...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/8493019/diff/7002/content/browser/plugin_servi...
content/browser/plugin_service.cc:83: return new
PluginService(webkit::npapi::PluginList::Singleton());
If you set the WillLoadPluginsCallback here, you don't need to reset it when
creating a TestPluginService.

[Robert Sesek, 2011-11-08 17:21:00]

http://codereview.chromium.org/8493019/diff/7002/content/browser/plugin_servi...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/8493019/diff/7002/content/browser/plugin_servi...
content/browser/plugin_service.cc:83: return new
PluginService(webkit::npapi::PluginList::Singleton());
On 2011/11/08 17:03:23, Bernhard Bauer wrote:
> If you set the WillLoadPluginsCallback here, you don't need to reset it when
> creating a TestPluginService.

Good idea. Done.

[jam, 2011-11-08 18:33:44]

ok, now that Dirk's change landed, this change seems more complicated than it
has to be. why not just have a ForTesting method that overrides the PluginList
to use, and call that directly in tests?

[Robert Sesek, 2011-11-08 19:07:48]

On 2011/11/08 18:33:44, John Abd-El-Malek wrote:
> ok, now that Dirk's change landed, this change seems more complicated than it
> has to be. why not just have a ForTesting method that overrides the PluginList
> to use, and call that directly in tests?

Because work is done in the ctor. What about this seems complicated?

[jam, 2011-11-08 21:09:18]

On 2011/11/08 19:07:48, rsesek wrote:
> On 2011/11/08 18:33:44, John Abd-El-Malek wrote:
> > ok, now that Dirk's change landed, this change seems more complicated than
it
> > has to be. why not just have a ForTesting method that overrides the
PluginList
> > to use, and call that directly in tests?
> 
> Because work is done in the ctor.

yet another reason why we should always aim for a simple constructor, and do all
the complexity in an Init function. this is actually the recommended style, but
as you can see it's not followed. in this case, 
 doing that would remove the need to change all the code that you did.
> What about this seems complicated?

the whole change seems unnecessary if we can set the PluginList before the other
code runs (per above)

[Robert Sesek, 2011-11-08 21:14:45]

On 2011/11/08 21:09:18, John Abd-El-Malek wrote:
> On 2011/11/08 19:07:48, rsesek wrote:
> > On 2011/11/08 18:33:44, John Abd-El-Malek wrote:
> > > ok, now that Dirk's change landed, this change seems more complicated than
> it
> > > has to be. why not just have a ForTesting method that overrides the
> PluginList
> > > to use, and call that directly in tests?
> > 
> > Because work is done in the ctor.
> 
> yet another reason why we should always aim for a simple constructor, and do
all
> the complexity in an Init function. this is actually the recommended style,
but
> as you can see it's not followed. in this case, 
>  doing that would remove the need to change all the code that you did.
> > What about this seems complicated?
> 
> the whole change seems unnecessary if we can set the PluginList before the
other
> code runs (per above)

Would you recommend I move the code into an Init member and post its execution
to the MessageLoop from the ctor?

[jam, 2011-11-08 21:34:30]

On Tue, Nov 8, 2011 at 1:14 PM, <rsesek@chromium.org> wrote:

> On 2011/11/08 21:09:18, John Abd-El-Malek wrote:
>
>> On 2011/11/08 19:07:48, rsesek wrote:
>> > On 2011/11/08 18:33:44, John Abd-El-Malek wrote:
>> > > ok, now that Dirk's change landed, this change seems more complicated
>> than
>> it
>> > > has to be. why not just have a ForTesting method that overrides the
>> PluginList
>> > > to use, and call that directly in tests?
>> >
>> > Because work is done in the ctor.
>>
>
>  yet another reason why we should always aim for a simple constructor, and
>> do
>>
> all
>
>> the complexity in an Init function. this is actually the recommended
>> style,
>>
> but
>
>> as you can see it's not followed. in this case,
>>  doing that would remove the need to change all the code that you did.
>> > What about this seems complicated?
>>
>
>  the whole change seems unnecessary if we can set the PluginList before the
>>
> other
>
>> code runs (per above)
>>
>
> Would you recommend I move the code into an Init member and post its
> execution
> to the MessageLoop from the ctor?
>

usually we just require the caller to call Init as well.

having the constructor post a task to an init is not a safe pattern and we
try to catch that in debug builds by releasing/addreffing first to make
sure that the ref count is not 0. in this case it's fine because it's a
singelton and it overrides the addref/release, but in general it's good to
not do this since others might copy it.

>
>
http://codereview.chromium.**org/8493019/<http://codereview.chromium.org/8493...
>

[Robert Sesek, 2011-11-08 22:47:33]

Patch set five breaks things out into an Init(). I prefer PS#4, though.

[jam, 2011-11-08 23:12:57]

http://codereview.chromium.org/8493019/diff/10003/content/browser/plugin_serv...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/8493019/diff/10003/content/browser/plugin_serv...
content/browser/plugin_service.cc:113: if (!service->did_init_)
we should know who creates this for the first time. i.e.
BrowserProcessImpl::CreateIOThread() should call Init() itself

http://codereview.chromium.org/8493019/diff/10003/content/test/test_plugin_se...
File content/test/test_plugin_service.cc (right):

http://codereview.chromium.org/8493019/diff/10003/content/test/test_plugin_se...
content/test/test_plugin_service.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
why do we need a testpluginservice?

the testing pluginlist that's used can always return true in
GetPluginsIfNoRefreshNeeded

[Robert Sesek, 2011-11-09 17:12:53]

Done. PTAL.

[jam, 2011-11-09 17:37:23]

http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:791:
plugin_service->Init(webkit::npapi::PluginList::Singleton());
this won't work in the component build (since the singleton will be different in
chrome vs content). so just make Init take 0 parameters, and in that function
grab webkit::npapi::PluginList::Singleton() if it hasn't been already set.

you'll want to have a method on PS as before to set the plugin list for testing,
i.e. SetPluginListForTesting(PluginList*

[Robert Sesek, 2011-11-09 17:44:10]

On 2011/11/09 17:37:23, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
> chrome/browser/browser_process_impl.cc:791:
> plugin_service->Init(webkit::npapi::PluginList::Singleton());
> this won't work in the component build (since the singleton will be different
in
> chrome vs content). so just make Init take 0 parameters, and in that function
> grab webkit::npapi::PluginList::Singleton() if it hasn't been already set.
> 
> you'll want to have a method on PS as before to set the plugin list for
testing,
> i.e. SetPluginListForTesting(PluginList*

With Dirk's refactoring, though, shouldn't there be no uses of
PluginList::Singleton in content/? If there are, can't it go through
PluginService::plugin_list()?

I'd much prefer to keep consistent dependency injection if possible.

[Robert Sesek, 2011-11-10 17:06:48]

On 2011/11/09 17:44:10, rsesek wrote:
> On 2011/11/09 17:37:23, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
> > File chrome/browser/browser_process_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
> > chrome/browser/browser_process_impl.cc:791:
> > plugin_service->Init(webkit::npapi::PluginList::Singleton());
> > this won't work in the component build (since the singleton will be
different
> in
> > chrome vs content). so just make Init take 0 parameters, and in that
function
> > grab webkit::npapi::PluginList::Singleton() if it hasn't been already set.
> > 
> > you'll want to have a method on PS as before to set the plugin list for
> testing,
> > i.e. SetPluginListForTesting(PluginList*
> 
> With Dirk's refactoring, though, shouldn't there be no uses of
> PluginList::Singleton in content/? If there are, can't it go through
> PluginService::plugin_list()?
> 
> I'd much prefer to keep consistent dependency injection if possible.

ping

[jam, 2011-11-10 17:13:12]

On 2011/11/10 17:06:48, rsesek wrote:
> On 2011/11/09 17:44:10, rsesek wrote:
> > On 2011/11/09 17:37:23, John Abd-El-Malek wrote:
> > >
> >
>
http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
> > > File chrome/browser/browser_process_impl.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/8493019/diff/13001/chrome/browser/browser_proc...
> > > chrome/browser/browser_process_impl.cc:791:
> > > plugin_service->Init(webkit::npapi::PluginList::Singleton());
> > > this won't work in the component build (since the singleton will be
> different
> > in
> > > chrome vs content). so just make Init take 0 parameters, and in that
> function
> > > grab webkit::npapi::PluginList::Singleton() if it hasn't been already set.
> > > 
> > > you'll want to have a method on PS as before to set the plugin list for
> > testing,
> > > i.e. SetPluginListForTesting(PluginList*
> > 
> > With Dirk's refactoring, though, shouldn't there be no uses of
> > PluginList::Singleton in content/? If there are, can't it go through
> > PluginService::plugin_list()?
> > 
> > I'd much prefer to keep consistent dependency injection if possible.
> 
> ping

(I was OOO yesterday)

the plan is to use DEPS to enforce that no one other than plugin_service.cc can
include plugin_list.h. This way we ensure that two copies of the singleton don't
exist. Since plugin_service.h obviously needs it, and we're trying to move to a
state where all the code goes through it to get to PluginList's methods, it's
better to avoid having the embedder also reach directly to PluginList.
Additionally, we really want to make things easy for embedders, so that they
don't even have to know about PluginList (i.e. ideally, they only have to know
about the Content API).

[Robert Sesek, 2011-11-10 18:50:30]

On 2011/11/10 17:13:12, John Abd-El-Malek wrote:
> (I was OOO yesterday)
> 
> the plan is to use DEPS to enforce that no one other than plugin_service.cc
can
> include plugin_list.h. This way we ensure that two copies of the singleton
don't
> exist. Since plugin_service.h obviously needs it, and we're trying to move to
a
> state where all the code goes through it to get to PluginList's methods, it's
> better to avoid having the embedder also reach directly to PluginList.
> Additionally, we really want to make things easy for embedders, so that they
> don't even have to know about PluginList (i.e. ideally, they only have to know
> about the Content API).

Fair enough. That's a pretty reasonable expectation for the content API. Done.

[jam, 2011-11-10 21:53:54]

lgtm

http://codereview.chromium.org/8493019/diff/25001/content/browser/plugin_serv...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/8493019/diff/25001/content/browser/plugin_serv...
content/browser/plugin_service.cc:138: DCHECK(!did_init_);
nit: is this member variable really needed now?

[Robert Sesek, 2011-11-10 22:04:32]

http://codereview.chromium.org/8493019/diff/25001/content/browser/plugin_serv...
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/8493019/diff/25001/content/browser/plugin_serv...
content/browser/plugin_service.cc:138: DCHECK(!did_init_);
On 2011/11/10 21:53:54, John Abd-El-Malek wrote:
> nit: is this member variable really needed now?

Doesn't have to be. Removed.

[commit-bot: I haz the power, 2011-11-10 22:05:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsesek@chromium.org/8493019/27001

[commit-bot: I haz the power, 2011-11-10 23:20:08]

Change committed as 109532