Rename PluginUpdater to PluginPrefs and make it per-profile.

Rename PluginUpdater to PluginPrefs and make it per-profile.

BUG=80794
TEST=none

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

[Bernhard Bauer, 2011-08-04 15:40:45]

Please review.

Elliot: ProfileKeyedService stuff
Evan: WebUI stuff

Thanks!

[Evan Stade, 2011-08-04 16:46:06]

I'm technically an owner of webui but I think it's probably better if you find
whoever originally authored these pages to do the review

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/ui/webui/plug...
File chrome/browser/ui/webui/plugins_ui.cc (right):

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/ui/webui/plug...
chrome/browser/ui/webui/plugins_ui.cc:277: Task* task =
get_plugins_factory_.NewRunnableMethod(
for example, I have no idea why there is a mixture of scoped and non-scoped
tasks here

[Elliot Glaysher, 2011-08-04 17:51:00]

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.cc
File chrome/browser/plugin_prefs.cc (right):

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.cc:14: #include "base/synchronization/lock.h"
I don't see you using a lock in this file. Left over?

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.cc:308: :
ProfileKeyedServiceFactory(ProfileDependencyManager::GetInstance()) {
I can't see any examples, but just in case: if your ProfileKeyedService depends
on any other services, make sure to declare them here (see
template_url_service_factory.cc for an example).

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.h
File chrome/browser/plugin_prefs.h (right):

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:36: class PluginPrefs : public
ProfileKeyedService,
Consider calling this PluginService, if to have a common naming convention with
everything else that's a ProfileKeyedService. (When I was doing my first run
through service-izing Profile components, I renamed stuff to BlahService.)

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:39: // This should be called before the first
profile is created.
Please mention that this will initialize the factory for dependency tracking
reasons; I didn't understand why the service needed static initialization at
first.

[Bernhard Bauer, 2011-08-05 13:42:19]

Trung, can you take a look at plugins_ui.cc?
Finnur, flash_ui.cc?
Thanks!

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.cc
File chrome/browser/plugin_prefs.cc (right):

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.cc:14: #include "base/synchronization/lock.h"
On 2011/08/04 17:51:00, Elliot Glaysher wrote:
> I don't see you using a lock in this file. Left over?

Yup. Gone.

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.cc:308: :
ProfileKeyedServiceFactory(ProfileDependencyManager::GetInstance()) {
On 2011/08/04 17:51:00, Elliot Glaysher wrote:
> I can't see any examples, but just in case: if your ProfileKeyedService
depends
> on any other services, make sure to declare them here (see
> template_url_service_factory.cc for an example).

Thanks for the heads-up! This one doesn't, though.

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.h
File chrome/browser/plugin_prefs.h (right):

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:36: class PluginPrefs : public
ProfileKeyedService,
On 2011/08/04 17:51:00, Elliot Glaysher wrote:
> Consider calling this PluginService, if to have a common naming convention
with
> everything else that's a ProfileKeyedService. (When I was doing my first run
> through service-izing Profile components, I renamed stuff to BlahService.)

There is already a singleton in content/ called PluginService :-/ My idea here
was to name it after ExtensionPrefs.

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs....
chrome/browser/plugin_prefs.h:39: // This should be called before the first
profile is created.
On 2011/08/04 17:51:00, Elliot Glaysher wrote:
> Please mention that this will initialize the factory for dependency tracking
> reasons; I didn't understand why the service needed static initialization at
> first.

Done.

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/ui/webui/plug...
File chrome/browser/ui/webui/plugins_ui.cc (right):

http://codereview.chromium.org/7564006/diff/5025/chrome/browser/ui/webui/plug...
chrome/browser/ui/webui/plugins_ui.cc:277: Task* task =
get_plugins_factory_.NewRunnableMethod(
On 2011/08/04 16:46:06, Evan Stade wrote:
> for example, I have no idea why there is a mixture of scoped and non-scoped
> tasks here

The ScopedRunnableMethodFactory task isn't executed when this object goes away,
but it's not threadsafe, so to jump to other threads we use NewRunnableFunction.

[Finnur, 2011-08-05 15:59:22]

flash_ui.cc LGTM.

On Fri, Aug 5, 2011 at 06:42, <bauerb@chromium.org> wrote:

> Trung, can you take a look at plugins_ui.cc?
> Finnur, flash_ui.cc?
> Thanks!
>
>
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/plugin_prefs.cc<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.cc>
> File chrome/browser/plugin_prefs.cc (right):
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/plugin_prefs.cc#**newcode14<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.cc#newcode14>
> chrome/browser/plugin_prefs.**cc:14: #include
> "base/synchronization/lock.h"
> On 2011/08/04 17:51:00, Elliot Glaysher wrote:
>
>> I don't see you using a lock in this file. Left over?
>>
>
> Yup. Gone.
>
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/plugin_prefs.cc#**newcode308<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.cc#newcode308>
> chrome/browser/plugin_prefs.**cc:308: :
> ProfileKeyedServiceFactory(**ProfileDependencyManager::**GetInstance()) {
> On 2011/08/04 17:51:00, Elliot Glaysher wrote:
>
>> I can't see any examples, but just in case: if your
>>
> ProfileKeyedService depends
>
>> on any other services, make sure to declare them here (see
>> template_url_service_factory.**cc for an example).
>>
>
> Thanks for the heads-up! This one doesn't, though.
>
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/plugin_prefs.h<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.h>
> File chrome/browser/plugin_prefs.h (right):
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/plugin_prefs.h#**newcode36<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.h#newcode36>
> chrome/browser/plugin_prefs.h:**36: class PluginPrefs : public
> ProfileKeyedService,
> On 2011/08/04 17:51:00, Elliot Glaysher wrote:
>
>> Consider calling this PluginService, if to have a common naming
>>
> convention with
>
>> everything else that's a ProfileKeyedService. (When I was doing my
>>
> first run
>
>> through service-izing Profile components, I renamed stuff to
>>
> BlahService.)
>
> There is already a singleton in content/ called PluginService :-/ My
> idea here was to name it after ExtensionPrefs.
>
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/plugin_prefs.h#**newcode39<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/plugin_prefs.h#newcode39>
> chrome/browser/plugin_prefs.h:**39: // This should be called before the
> first profile is created.
> On 2011/08/04 17:51:00, Elliot Glaysher wrote:
>
>> Please mention that this will initialize the factory for dependency
>>
> tracking
>
>> reasons; I didn't understand why the service needed static
>>
> initialization at
>
>> first.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/ui/webui/plugins_ui.cc<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/ui/webui/plugins_ui.cc>
> File chrome/browser/ui/webui/**plugins_ui.cc (right):
>
> http://codereview.chromium.**org/7564006/diff/5025/chrome/**
>
browser/ui/webui/plugins_ui.**cc#newcode277<http://codereview.chromium.org/7564006/diff/5025/chrome/browser/ui/webui/plugins_ui.cc#newcode277>
> chrome/browser/ui/webui/**plugins_ui.cc:277: Task* task =
> get_plugins_factory_.**NewRunnableMethod(
> On 2011/08/04 16:46:06, Evan Stade wrote:
>
>> for example, I have no idea why there is a mixture of scoped and
>>
> non-scoped
>
>> tasks here
>>
>
> The ScopedRunnableMethodFactory task isn't executed when this object
> goes away, but it's not threadsafe, so to jump to other threads we use
> NewRunnableFunction.
>
>
>
http://codereview.chromium.**org/7564006/<http://codereview.chromium.org/7564...
>

[Bernhard Bauer, 2011-08-09 15:27:45]

Ping?

Note that I made PluginPrefs refcounted again (sorry for flip-flopping here,
I've got couple of other CLs in flight on top of this one, so the requirements
are changing).

Dmitry, could you take a quick look at login_utils.cc? I think it should be fine
now that PluginPrefs is a per-profile object.

[Dmitry Polukhin, 2011-08-09 15:49:51]

I agree changes in login_utils.cc should be fine now.

[Elliot Glaysher, 2011-08-09 17:53:29]

Why did you remove the initialization in profile_dependency_manager.cc?
Even if there are no dependencies now, I want to keep track of everything that's
been PKSFized.

[Bernhard Bauer, 2011-08-10 09:04:44]

On 2011/08/09 17:53:29, Elliot Glaysher wrote:
> Why did you remove the initialization in profile_dependency_manager.cc?
> Even if there are no dependencies now, I want to keep track of everything
that's
> been PKSFized.

Oh, sorry. I read the comment above it as saying that it's undesirable to put
initialization of every factory there :-)

[Elliot Glaysher, 2011-08-10 19:06:37]

LGTM after adding initialization back

http://codereview.chromium.org/7564006/diff/20001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/7564006/diff/20001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_dependency_manager.cc:43:
PluginPrefs::Initialize();
nit: alphabetize