Move PromoResourceService from Profile to BrowserProcessImpl/local_state().

Move PromoResourceService from Profile to BrowserProcessImpl/local_state().

TBR=rsesek@chromium.org
BUG=120996, 143773
TEST=tests pass, we still get promos, less crud on profile.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175103

[Dan Beam, 2012-12-28 05:38:22]

rlp@/achuith@: can you see if this is the right direction before I go much
further?

[rpetterson, 2012-12-28 17:40:39]

On 2012/12/28 05:38:22, Dan Beam wrote:
> rlp@/achuith@: can you see if this is the right direction before I go much
> further?

This LGTM from a profiles standpoint since if I understand correctly you're just
transferring ownership to the browser process.

[Dan Beam, 2012-12-28 17:43:51]

+rsesek@/aruslan@ as FYI

[aruslan, 2012-12-28 18:07:16]

LGTM; if there would be issues on Android I'll fix them.
Adding msarda@ for iOS FYI.

[Dan Beam, 2012-12-29 02:36:32]

OK, well, there doesn't seem to be much objection to this so far, so let's bring
on the reviewers:

estade@: chrome/browser/ui/webui/ntp/
jcivelli@: chrome/browser/ui/webui/ntp/android/, chrome/test/
achuith@: chrome/browser/web_resource/
bauerb@: chrome/browser/preferences/
jam@: chrome/browser/browser_process_impl.*

[Dan Beam, 2012-12-29 02:37:36]

whoops, jam@/estade@/bauerb@, see
https://chromiumcodereview.appspot.com/11689004/#msg5

[Dan Beam, 2012-12-29 02:48:05]

(and jcivelli@ as well)

[msarda, 2012-12-31 08:58:34]

LGTM - I'll handle the iOS issues if any.

@aruslan: Thank you for adding me to this CL!

[Bernhard Bauer, 2012-12-31 11:51:52]

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/brow...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/brow...
chrome/browser/browser_process_impl.cc:844: !promo_resource_service_) {
When can it happen that |promo_resource_service_| is not NULL here?

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:316: new
base::DictionaryValue);
Nit: an empty dictionary is the default value for RegisterDictionaryPref, so
it's not necessary explicitly.

[Dan Beam, 2013-01-02 04:36:34]

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/brow...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/brow...
chrome/browser/browser_process_impl.cc:844: !promo_resource_service_) {
On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> When can it happen that |promo_resource_service_| is not NULL here? 

I am not sure, I was just following the plugins service example above. Perhaps
jam@ will know.

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:316: new
base::DictionaryValue);
On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> Nit: an empty dictionary is the default value for RegisterDictionaryPref, so
> it's not necessary explicitly. 

OK, should I remove it from the below user pref registration as well, then?

[Bernhard Bauer, 2013-01-02 10:57:43]

LGTM with some cleanups:

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/brow...
File chrome/browser/browser_process_impl.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/brow...
chrome/browser/browser_process_impl.cc:844: !promo_resource_service_) {
On 2013/01/02 04:36:35, Dan Beam wrote:
> On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > When can it happen that |promo_resource_service_| is not NULL here? 
> 
> I am not sure, I was just following the plugins service example above. Perhaps
> jam@ will know.

It's probably unnecessary there as well. Feel free to remove it.

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/web_...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/3007/chrome/browser/web_...
chrome/browser/web_resource/notification_promo.cc:316: new
base::DictionaryValue);
On 2013/01/02 04:36:35, Dan Beam wrote:
> On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > Nit: an empty dictionary is the default value for RegisterDictionaryPref, so
> > it's not necessary explicitly. 
> 
> OK, should I remove it from the below user pref registration as well, then?

Please and thank you! :)

[jam, 2013-01-02 15:59:00]

lgtm for the two files with nits

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:844: !promo_resource_service_) {
On 2013/01/02 04:36:35, Dan Beam wrote:
> On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > When can it happen that |promo_resource_service_| is not NULL here? 
> 
> I am not sure, I was just following the plugins service example above. Perhaps
> jam@ will know.

I don't see where pluginservice is null checked? it looks like it's used
directly at line 798. i agree take the null check for promo_resource_service_
out.

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:282: scoped_refptr<PromoResourceService>
promo_resource_service_;
nit: in general, it's more readable if all the non-ifdef'd members are listed
first, then all the ifdefs start. it looks like this is what this file was
adhering to as well. so move this up to before lie 261

[Jay Civelli, 2013-01-02 16:58:38]

test change LGTM

[Dan Beam, 2013-01-02 18:10:31]

for jam@/bauerb@

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:844: !promo_resource_service_) {
On 2013/01/02 15:59:00, John Abd-El-Malek wrote:
> On 2013/01/02 04:36:35, Dan Beam wrote:
> > On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > > When can it happen that |promo_resource_service_| is not NULL here? 
> > 
> > I am not sure, I was just following the plugins service example above.
Perhaps
> > jam@ will know.
> 
> I don't see where pluginservice is null checked? it looks like it's used
> directly at line 798. i agree take the null check for promo_resource_service_
> out.

sorry, I meant L837:

  if (!plugins_resource_service_) {
    plugins_resource_service_ = new PluginsResourceService(local_state());
    plugins_resource_service_->StartAfterDelay();
  }

[jam, 2013-01-02 18:14:54]

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.cc:844: !promo_resource_service_) {
On 2013/01/02 18:10:32, Dan Beam wrote:
> On 2013/01/02 15:59:00, John Abd-El-Malek wrote:
> > On 2013/01/02 04:36:35, Dan Beam wrote:
> > > On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > > > When can it happen that |promo_resource_service_| is not NULL here? 
> > > 
> > > I am not sure, I was just following the plugins service example above.
> Perhaps
> > > jam@ will know.
> > 
> > I don't see where pluginservice is null checked? it looks like it's used
> > directly at line 798. i agree take the null check for
promo_resource_service_
> > out.
> 
> sorry, I meant L837:
> 
>   if (!plugins_resource_service_) {
>     plugins_resource_service_ = new PluginsResourceService(local_state());
>     plugins_resource_service_->StartAfterDelay();
>   }

that seems to be the only place that creates PluginResourceService, so that's
legitimate code. the null check there seems even unnecessary since it should
always be null when it gets here...

[Evan Stade, 2013-01-02 22:08:43]

https://codereview.chromium.org/11689004/diff/28002/chrome/browser/web_resour...
File chrome/browser/web_resource/notification_promo.h (left):

https://codereview.chromium.org/11689004/diff/28002/chrome/browser/web_resour...
chrome/browser/web_resource/notification_promo.h:97: PrefService* prefs_;
why did you change this to a PrefServiceSimple?

[Dan Beam, 2013-01-02 22:27:51]

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
chrome/browser/browser_process_impl.h:282: scoped_refptr<PromoResourceService>
promo_resource_service_;
On 2013/01/02 15:59:00, John Abd-El-Malek wrote:
> nit: in general, it's more readable if all the non-ifdef'd members are listed
> first, then all the ifdefs start. it looks like this is what this file was
> adhering to as well. so move this up to before lie 261

Done.

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/web_resourc...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/11689004/diff/3007/chrome/browser/web_resourc...
chrome/browser/web_resource/notification_promo.cc:316: new
base::DictionaryValue);
On 2013/01/02 10:57:44, Bernhard Bauer wrote:
> On 2013/01/02 04:36:35, Dan Beam wrote:
> > On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > > Nit: an empty dictionary is the default value for RegisterDictionaryPref,
so
> > > it's not necessary explicitly. 
> > 
> > OK, should I remove it from the below user pref registration as well, then?
> 
> Please and thank you! :)

Done.

https://codereview.chromium.org/11689004/diff/28002/chrome/browser/web_resour...
File chrome/browser/web_resource/notification_promo.h (left):

https://codereview.chromium.org/11689004/diff/28002/chrome/browser/web_resour...
chrome/browser/web_resource/notification_promo.h:97: PrefService* prefs_;
On 2013/01/02 22:08:43, Evan Stade wrote:
> why did you change this to a PrefServiceSimple?

To ensure all calling code is registering their prefs with local_state().

The ctor was explicit so interchanging between a PrefService*,
PrefServiceSimple*, and PrefServiceSyncable* wasn't allowed - should I change to
PrefService* and remove explicit?  I also originally thought this adds the
tiniest bit of type safety for people mixing user prefs with local state prefs
and this guarded against that, though I've noticed many comments in the code
that say "TODO(name): just return a PrefService*"

[Evan Stade, 2013-01-02 22:56:13]

On 2013/01/02 22:27:51, Dan Beam wrote:
>
https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
> File chrome/browser/browser_process_impl.h (right):
> 
>
https://codereview.chromium.org/11689004/diff/3007/chrome/browser/browser_pro...
> chrome/browser/browser_process_impl.h:282: scoped_refptr<PromoResourceService>
> promo_resource_service_;
> On 2013/01/02 15:59:00, John Abd-El-Malek wrote:
> > nit: in general, it's more readable if all the non-ifdef'd members are
listed
> > first, then all the ifdefs start. it looks like this is what this file was
> > adhering to as well. so move this up to before lie 261
> 
> Done.
> 
>
https://codereview.chromium.org/11689004/diff/3007/chrome/browser/web_resourc...
> File chrome/browser/web_resource/notification_promo.cc (right):
> 
>
https://codereview.chromium.org/11689004/diff/3007/chrome/browser/web_resourc...
> chrome/browser/web_resource/notification_promo.cc:316: new
> base::DictionaryValue);
> On 2013/01/02 10:57:44, Bernhard Bauer wrote:
> > On 2013/01/02 04:36:35, Dan Beam wrote:
> > > On 2012/12/31 11:51:52, Bernhard Bauer wrote:
> > > > Nit: an empty dictionary is the default value for
RegisterDictionaryPref,
> so
> > > > it's not necessary explicitly. 
> > > 
> > > OK, should I remove it from the below user pref registration as well,
then?
> > 
> > Please and thank you! :)
> 
> Done.
> 
>
https://codereview.chromium.org/11689004/diff/28002/chrome/browser/web_resour...
> File chrome/browser/web_resource/notification_promo.h (left):
> 
>
https://codereview.chromium.org/11689004/diff/28002/chrome/browser/web_resour...
> chrome/browser/web_resource/notification_promo.h:97: PrefService* prefs_;
> On 2013/01/02 22:08:43, Evan Stade wrote:
> > why did you change this to a PrefServiceSimple?
> 
> To ensure all calling code is registering their prefs with local_state().
> 
> The ctor was explicit so interchanging between a PrefService*,
> PrefServiceSimple*, and PrefServiceSyncable* wasn't allowed - should I change
to
> PrefService* and remove explicit?  I also originally thought this adds the
> tiniest bit of type safety for people mixing user prefs with local state prefs
> and this guarded against that, though I've noticed many comments in the code
> that say "TODO(name): just return a PrefService*"

can you make the member var and all the function signatures everywhere a
PrefService, but the constructor as is for the type safety you described?
(although it will be changed when that TODO you quoted is fixed)

[Evan Stade, 2013-01-03 05:01:08]

thanks lgtm

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service.cc:96: // For testing.
put this comment in the header only.

[Dan Beam, 2013-01-03 05:05:47]

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service.cc:106:
ScheduleNotificationOnInitForTesting(prefs);
this is triggering a presubmit warning, I'll see if I can fix that.

[achuithb, 2013-01-03 05:42:51]

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service.cc:106:
ScheduleNotificationOnInitForTesting(prefs);
On 2013/01/03 05:05:47, Dan Beam wrote:
> this is triggering a presubmit warning, I'll see if I can fix that.

This particular presubmit warning is not reliable, so your code is probably ok.

[Dan Beam, 2013-01-03 09:08:21]

achuith@: btw, can you do a chrome/browser/web_resource/OWNERS review? I asked
earlier on, but it might've been lost in the noise.

[Dan Beam, 2013-01-03 21:27:10]

+rsesek for chrome/browser/web_resource (didn't notice that achuith@ is OOO)

[Robert Sesek, 2013-01-03 21:40:10]

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/notification_promo.cc:195: :
prefs_(g_browser_process->local_state()),
Can you dependency inject this, potentially allowing you to remove the
ForTesting methods?

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/notification_promo.h (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/notification_promo.h:82: friend class
NotificationPromoTest;
friend declarations come first

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service.h (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service.h:39: friend class
PromoResourceServiceTest;
friends come first

[Dan Beam, 2013-01-03 22:47:37]

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/notification_promo.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/notification_promo.cc:195: :
prefs_(g_browser_process->local_state()),
On 2013/01/03 21:40:10, rsesek wrote:
> Can you dependency inject this, potentially allowing you to remove the
> ForTesting methods?

I was doing that before but estade@ and I thought a little awkwardness for
testing was an acceptable tradeoff to remove this from all the calling code's
API.

We talked about this over chat and TestingBrowserProcess::SetLocalState() seemed
to do the trick.

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/notification_promo.h (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/notification_promo.h:82: friend class
NotificationPromoTest;
On 2013/01/03 21:40:10, rsesek wrote:
> friend declarations come first

Done. (removed *Internal methods)

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service.h (right):

https://chromiumcodereview.appspot.com/11689004/diff/30005/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service.h:39: friend class
PromoResourceServiceTest;
On 2013/01/03 21:40:10, rsesek wrote:
> friends come first

Done. (not needed any more)

[Robert Sesek, 2013-01-04 01:16:13]

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:29:
TestingBrowserProcess::GetGlobal()->SetLocalState(local_state_.Get());
This needs to be NULLed in the dtor, at least I think so based on the comments.

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service_unittest.cc:31:
TestingBrowserProcess::GetGlobal()->SetLocalState(local_state_.Get());
Same

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/test/base/t...
File chrome/test/base/testing_browser_process.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/test/base/t...
chrome/test/base/testing_browser_process.cc:34: TestingBrowserProcess*
TestingBrowserProcess::GetGlobal() {
I'm not sure this is right. Also, who's setting up g_browser_process?

[Dan Beam, 2013-01-04 01:34:41]

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/browser/web...
File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc
(right):

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/browser/web...
chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:29:
TestingBrowserProcess::GetGlobal()->SetLocalState(local_state_.Get());
On 2013/01/04 01:16:13, rsesek wrote:
> This needs to be NULLed in the dtor, at least I think so based on the
comments.

ScopedTestingLocalState both does that and DCHECKs() that you don't do it
yourself.

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/test/base/t...
File chrome/test/base/testing_browser_process.cc (right):

https://chromiumcodereview.appspot.com/11689004/diff/30034/chrome/test/base/t...
chrome/test/base/testing_browser_process.cc:34: TestingBrowserProcess*
TestingBrowserProcess::GetGlobal() {
On 2013/01/04 01:16:13, rsesek wrote:
> I'm not sure this is right. Also, who's setting up g_browser_process?

I talked with jcivelli@ about this (a chrome/test OWNER) and he was OK with it
(and updating everywhere else).  g_browser_process comes up in various places
(chrome/browser/browser_process_impl.cc, chrome/test/base/chrome_test_suite.cc),
but this really doesn't change anything about how the current code works, it
just makes the calling code prettier.

[Dan Beam, 2013-01-04 01:43:56]

OK, moved that method to a different CL.  I'll address anything else in a follow
up CL.

[msarda, 2013-01-04 09:54:38]

https://codereview.chromium.org/11689004/diff/27028/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11689004/diff/27028/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:791: if
(local_state_->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
Please note that the pushed version does not match the review here - you have
duplicated the calls to ApplyDefaultBrowserPolicy(). The pushed version (as
shown at
https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/browser_proce...)
:

  if (local_state_->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
    ApplyDefaultBrowserPolicy();

#if defined(ENABLE_PLUGINS)
  PluginService* plugin_service = PluginService::GetInstance();
  plugin_service->SetFilter(ChromePluginServiceFilter::GetInstance());
  plugin_service->StartWatchingPlugins();

#if defined(OS_POSIX)
  // Also find plugins in a user-specific plugins dir,
  // e.g. ~/.config/chromium/Plugins.
  FilePath user_data_dir;
  if (PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
    plugin_service->AddExtraPluginDir(user_data_dir.Append("Plugins"));
#endif

#endif  // defined(ENABLE_PLUGINS)

  if (local_state()->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
    ApplyDefaultBrowserPolicy();

[Dan Beam, 2013-01-04 17:44:57]

On 2013/01/04 09:54:38, msarda wrote:
>
https://codereview.chromium.org/11689004/diff/27028/chrome/browser/browser_pr...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
https://codereview.chromium.org/11689004/diff/27028/chrome/browser/browser_pr...
> chrome/browser/browser_process_impl.cc:791: if
> (local_state_->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
> Please note that the pushed version does not match the review here - you have
> duplicated the calls to ApplyDefaultBrowserPolicy(). The pushed version (as
> shown at
>
https://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/browser_proce...)
> :
> 
>   if (local_state_->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
>     ApplyDefaultBrowserPolicy();
> 
> #if defined(ENABLE_PLUGINS)
>   PluginService* plugin_service = PluginService::GetInstance();
>   plugin_service->SetFilter(ChromePluginServiceFilter::GetInstance());
>   plugin_service->StartWatchingPlugins();
> 
> #if defined(OS_POSIX)
>   // Also find plugins in a user-specific plugins dir,
>   // e.g. ~/.config/chromium/Plugins.
>   FilePath user_data_dir;
>   if (PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
>     plugin_service->AddExtraPluginDir(user_data_dir.Append("Plugins"));
> #endif
> 
> #endif  // defined(ENABLE_PLUGINS)
> 
>   if
(local_state()->IsManagedPreference(prefs::kDefaultBrowserSettingEnabled))
>     ApplyDefaultBrowserPolicy();

msarda@: Thanks for the heads up! That was a bad merge on my part and I think
was fixed in http://crrev.com/175105 (only a couple minutes after the original
CL).  I'll remove the last 2 patchsets, as that's not the committed diff for
this issue.

[Robert Sesek, 2013-01-04 17:48:18]

LGTM

[achuithb, 2013-01-08 21:57:33]

lgtm

https://codereview.chromium.org/11689004/diff/39007/chrome/browser/web_resour...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/11689004/diff/39007/chrome/browser/web_resour...
chrome/browser/web_resource/notification_promo.cc:321: // TODO(dbeam): Remove in
M28 when we're reasonably sure all prefs are gone.
Should probably file a bug to track this and add it to the comment.

https://codereview.chromium.org/11689004/diff/39007/chrome/browser/web_resour...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://codereview.chromium.org/11689004/diff/39007/chrome/browser/web_resour...
chrome/browser/web_resource/promo_resource_service.cc:71: // TODO(dbeam): remove
in M28 when all prefs have been cleared.
Same.