Use PrefChangeRegistrar everywhere

Use PrefChangeRegistrar everywhere

BUG=54955
TEST=PrefChangeRegistrarTest.*

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

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

[danno, 2010-09-10 14:17:18]

Please review.

[Mattias Nissler (ping if slow), 2010-09-10 15:50:35]

Overall looks good, I put some suggestions and pointed out some possible issues
that might need checking (maybe you already did).

http://codereview.chromium.org/3304015/diff/7003/3050
File chrome/browser/dom_ui/core_options_handler.cc (left):

http://codereview.chromium.org/3304015/diff/7003/3050#oldcode77
chrome/browser/dom_ui/core_options_handler.cc:77: void
CoreOptionsHandler::Uninitialize() {
As far as I can see, this is the last implementation of Uninitialize(), so you
should probably remove this method entirely once you have double-checked it's
not needed anymore.

Edit: It seems the CoreChromeOSOptionsHandler subclass requires the
StopObservingPref() class. Are you aware of that?

http://codereview.chromium.org/3304015/diff/7003/3050
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3050#newcode247
chrome/browser/dom_ui/core_options_handler.cc:247:
UTF16ToWideHack(callback_func_name)));
Indentation problem

http://codereview.chromium.org/3304015/diff/7003/3056
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3056#newcode874
chrome/browser/extensions/extensions_service.cc:874:
pref_change_registrar_.Remove(prefs::kExtensionInstallDenyList, this);
This doesn't make much difference from what we had before. Maybe add Clear()
method to the registrar?

http://codereview.chromium.org/3304015/diff/7003/3058
File chrome/browser/host_content_settings_map.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3058#newcode923
chrome/browser/host_content_settings_map.cc:923:
pref_change_registrar_.Remove(prefs::kBlockNonsandboxedPlugins, this);
Clear() comment also applies here.

http://codereview.chromium.org/3304015/diff/7003/3060
File chrome/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3060#newcode133
chrome/browser/host_zoom_map.cc:133:
pref_change_registrar_.Remove(prefs::kPerHostZoomLevels, this);
and here.

http://codereview.chromium.org/3304015/diff/7003/3063
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3063#newcode660
chrome/browser/net/chrome_url_request_context.cc:660: prefs_ = NULL;
And a Clear() here. And if you add Empty(), you can nuke the |prefs_| member.

http://codereview.chromium.org/3304015/diff/7003/3065
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3065#newcode267
chrome/browser/notifications/desktop_notification_service.cc:267:
registrar_.Remove(prefs::kDesktopNotificationDeniedOrigins, this);
Clear()

http://codereview.chromium.org/3304015/diff/7003/3068
File chrome/browser/prefs/pref_change_registrar.h (right):

http://codereview.chromium.org/3304015/diff/7003/3068#newcode43
chrome/browser/prefs/pref_change_registrar.h:43: void RemoveAll();
Oh, so Clear() is called RemoveAll(). I'm fine with that :)

http://codereview.chromium.org/3304015/diff/7003/3069
File chrome/browser/prefs/pref_change_registrar_unittest.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3069#newcode75
chrome/browser/prefs/pref_change_registrar_unittest.cc:75: // Explicitly check
the expectations now to make sure that the RemoveAll
s/RemoveAll/Remove calls/

http://codereview.chromium.org/3304015/diff/7003/3070
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/3304015/diff/7003/3070#newcode237
chrome/browser/prefs/pref_service.h:237: virtual void RemovePrefObserver(const
char* path, NotificationObserver* obs);
So you think it's a good idea to force people to use the new stuff? I'm not
really sure, but I don't mind either.

http://codereview.chromium.org/3304015/diff/7003/3074
File chrome/browser/profile_impl.cc (left):

http://codereview.chromium.org/3304015/diff/7003/3074#oldcode475
chrome/browser/profile_impl.cc:475:
prefs->RemovePrefObserver(prefs::kEnableAutoSpellCorrect, this);
Are you sure it's OK to remove these registrations after |spellcheck_host_| is
teared down below?

http://codereview.chromium.org/3304015/diff/7003/3076
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3076#newcode214
chrome/browser/sync/glue/preference_change_processor.cc:214:
registrar_.Add((*it).c_str(), this);
That's at least the second occurrence of the Start()/Stop() pattern. Maybe it's
worth to support that directly in the registrar?

[danno, 2010-09-10 16:28:40]

http://codereview.chromium.org/3304015/diff/7003/3050
File chrome/browser/dom_ui/core_options_handler.cc (left):

http://codereview.chromium.org/3304015/diff/7003/3050#oldcode77
chrome/browser/dom_ui/core_options_handler.cc:77: void
CoreOptionsHandler::Uninitialize() {
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> As far as I can see, this is the last implementation of Uninitialize(), so you
> should probably remove this method entirely once you have double-checked it's
> not needed anymore.
> 
> Edit: It seems the CoreChromeOSOptionsHandler subclass requires the
> StopObservingPref() class. Are you aware of that?

To be on the really safe side, I put this method back and kept the logic
identical to how it was before. It's not pretty, but I hope that I make sure not
to break anything that way.

http://codereview.chromium.org/3304015/diff/7003/3050
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3050#newcode247
chrome/browser/dom_ui/core_options_handler.cc:247:
UTF16ToWideHack(callback_func_name)));
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> Indentation problem

Done.

http://codereview.chromium.org/3304015/diff/7003/3056
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3056#newcode874
chrome/browser/extensions/extensions_service.cc:874:
pref_change_registrar_.Remove(prefs::kExtensionInstallDenyList, this);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> This doesn't make much difference from what we had before. Maybe add Clear()
> method to the registrar?

Done.

http://codereview.chromium.org/3304015/diff/7003/3058
File chrome/browser/host_content_settings_map.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3058#newcode923
chrome/browser/host_content_settings_map.cc:923:
pref_change_registrar_.Remove(prefs::kBlockNonsandboxedPlugins, this);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> Clear() comment also applies here.

Done.

http://codereview.chromium.org/3304015/diff/7003/3060
File chrome/browser/host_zoom_map.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3060#newcode133
chrome/browser/host_zoom_map.cc:133:
pref_change_registrar_.Remove(prefs::kPerHostZoomLevels, this);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> and here.

Done.

http://codereview.chromium.org/3304015/diff/7003/3063
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3063#newcode660
chrome/browser/net/chrome_url_request_context.cc:660: prefs_ = NULL;
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> And a Clear() here. And if you add Empty(), you can nuke the |prefs_| member.

Done, but prefs_ not removed. there a DCHECK that uses it in the destructor.

http://codereview.chromium.org/3304015/diff/7003/3065
File chrome/browser/notifications/desktop_notification_service.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3065#newcode267
chrome/browser/notifications/desktop_notification_service.cc:267:
registrar_.Remove(prefs::kDesktopNotificationDeniedOrigins, this);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> Clear()

Done.

http://codereview.chromium.org/3304015/diff/7003/3068
File chrome/browser/prefs/pref_change_registrar.h (right):

http://codereview.chromium.org/3304015/diff/7003/3068#newcode43
chrome/browser/prefs/pref_change_registrar.h:43: void RemoveAll();
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> Oh, so Clear() is called RemoveAll(). I'm fine with that :)

Done.

http://codereview.chromium.org/3304015/diff/7003/3069
File chrome/browser/prefs/pref_change_registrar_unittest.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3069#newcode75
chrome/browser/prefs/pref_change_registrar_unittest.cc:75: // Explicitly check
the expectations now to make sure that the RemoveAll
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> s/RemoveAll/Remove calls/

Done.

http://codereview.chromium.org/3304015/diff/7003/3070
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/3304015/diff/7003/3070#newcode237
chrome/browser/prefs/pref_service.h:237: virtual void RemovePrefObserver(const
char* path, NotificationObserver* obs);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> So you think it's a good idea to force people to use the new stuff? I'm not
> really sure, but I don't mind either.

Done.

http://codereview.chromium.org/3304015/diff/7003/3074
File chrome/browser/profile_impl.cc (left):

http://codereview.chromium.org/3304015/diff/7003/3074#oldcode475
chrome/browser/profile_impl.cc:475:
prefs->RemovePrefObserver(prefs::kEnableAutoSpellCorrect, this);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> Are you sure it's OK to remove these registrations after |spellcheck_host_| is
> teared down below?

Done.

http://codereview.chromium.org/3304015/diff/7003/3076
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3076#newcode214
chrome/browser/sync/glue/preference_change_processor.cc:214:
registrar_.Add((*it).c_str(), this);
On 2010/09/10 15:50:35, Mattias Nissler wrote:
> That's at least the second occurrence of the Start()/Stop() pattern. Maybe
it's
> worth to support that directly in the registrar?

Maybe, but in another CL. I wanted this change to be pretty much mechanical so
that it is low risk.

[Mattias Nissler (ping if slow), 2010-09-10 16:49:12]

LGTM pending the prefs_ removal.

http://codereview.chromium.org/3304015/diff/7003/3063
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/3304015/diff/7003/3063#newcode660
chrome/browser/net/chrome_url_request_context.cc:660: prefs_ = NULL;
On 2010/09/10 16:28:41, danno wrote:
> On 2010/09/10 15:50:35, Mattias Nissler wrote:
> > And a Clear() here. And if you add Empty(), you can nuke the |prefs_|
member.
> 
> Done, but prefs_ not removed. there a DCHECK that uses it in the destructor.

Yeah, but you can replace that DCHECK with an equivalent one that uses the
to-be-introduced Empty() function I mentioned above.

[Pam (message me for reviews), 2010-09-12 08:29:39]

Zoom, drive-by review.

- Pam

http://codereview.chromium.org/3304015/diff/9002/50027
File chrome/browser/prefs/pref_change_registrar_unittest.cc (right):

http://codereview.chromium.org/3304015/diff/9002/50027#newcode77
chrome/browser/prefs/pref_change_registrar_unittest.cc:77:
Mock::VerifyAndClearExpectations(service());
How about checking registrar.IsEmpty() (instead or also)?

http://codereview.chromium.org/3304015/diff/9002/50027#newcode114
chrome/browser/prefs/pref_change_registrar_unittest.cc:114:
Mock::VerifyAndClearExpectations(service());
ditto

http://codereview.chromium.org/3304015/diff/9002/50028
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/3304015/diff/9002/50028#newcode230
chrome/browser/prefs/pref_service.h:230: friend class subtle::PrefMemberBase;
Friend classes set off my "hmm" sensor. I admit I haven't examined these
closely, but if they're only used in testing, please add a comment saying so, or
consider exposing a getter that would allow tests the access they need. If
they're used for more than tests, consider holding a reference to the class
object(s) instead.

[danno, 2010-09-21 11:47:09]

Pam, can you please take a quick look at this again? Mattias is on vacation, and
in order to get all the builds building  tests passing I had to tweak the code a
bit more. Since I already had Mattias' LGTM, you only need to review files that
have changed since patch set 9.

Thanks, Danno

http://codereview.chromium.org/3304015/diff/9002/50027
File chrome/browser/prefs/pref_change_registrar_unittest.cc (right):

http://codereview.chromium.org/3304015/diff/9002/50027#newcode77
chrome/browser/prefs/pref_change_registrar_unittest.cc:77:
Mock::VerifyAndClearExpectations(service());
On 2010/09/12 08:29:40, Pam wrote:
> How about checking registrar.IsEmpty() (instead or also)?

Done.

http://codereview.chromium.org/3304015/diff/9002/50027#newcode114
chrome/browser/prefs/pref_change_registrar_unittest.cc:114:
Mock::VerifyAndClearExpectations(service());
On 2010/09/12 08:29:40, Pam wrote:
> ditto

Done.

http://codereview.chromium.org/3304015/diff/9002/50028
File chrome/browser/prefs/pref_service.h (right):

http://codereview.chromium.org/3304015/diff/9002/50028#newcode230
chrome/browser/prefs/pref_service.h:230: friend class subtle::PrefMemberBase;
I completely understand your feeling here. In this case, I think the usage is
warranted, and Mattias agreed talking offline. I have added comments explaining
exactly why the friend is necessary.

On 2010/09/12 08:29:40, Pam wrote:
> Friend classes set off my "hmm" sensor. I admit I haven't examined these
> closely, but if they're only used in testing, please add a comment saying so,
or
> consider exposing a getter that would allow tests the access they need. If
> they're used for more than tests, consider holding a reference to the class
> object(s) instead.

[Paweł Hajdan Jr., 2010-09-21 11:59:03]

Drive-by with a test comment.

Would it be possible to expose a CreateExtensionsService in TestingProfile
(possibly with params to pass to its ctor) instead of a SetExtensionsService? I
think that it's quite easy to forget about SetExtensionsService compared to
CreateExtensionsService.

[Pam (message me for reviews), 2010-09-21 13:38:27]

LGTM. I have no strong opinion on Paweł's suggestion -- either way is OK with
me.

- Pam

[danno, 2010-09-21 14:34:25]

I agree, this is annoying and isn't perfect, but there's a chicken-and-egg
problem here. The ExtensionService needs to have a pointer to the profile in its
constructor, so it has to be fully constructed before the ExtensionService is
created. I'm going to leave as-is.

On 2010/09/21 11:59:03, Paweł Hajdan Jr. wrote:
> Drive-by with a test comment.
> 
> Would it be possible to expose a CreateExtensionsService in TestingProfile
> (possibly with params to pass to its ctor) instead of a SetExtensionsService?
I
> think that it's quite easy to forget about SetExtensionsService compared to
> CreateExtensionsService.

[danno, 2010-09-21 15:43:12]

Pawel, can you take a look at changes since revision 12? Your suggestion ended
up being a good one.

[Paweł Hajdan Jr., 2010-09-21 16:01:14]

Code I commented in the drive-by LGTM with one nit. Thanks!

http://codereview.chromium.org/3304015/diff/62003/60050
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/3304015/diff/62003/60050#newcode90
chrome/test/testing_profile.h:90: // Creates an ExtensionsService initialzed
with the testing profile and
nit: typo - initialized

[danno, 2010-09-24 12:40:00]

a few more tweaks necessary to make sure linux tests don't crash. Pawel, can you
take a look at the changes since 16?

[Paweł Hajdan Jr., 2010-09-24 15:01:21]

LGTM

[danno, 2010-09-29 07:56:48]

@Pawel: sorry about this, one more look, please. I finally found the crasher.
Just take a quick look at extension_toolbar_model.h/cc and the changes to
extension_service.cc in patch set 19. Thanks!

[Paweł Hajdan Jr., 2010-09-29 09:18:13]

LGTMzored

http://codereview.chromium.org/3304015/diff/115001/90019
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3304015/diff/115001/90019#newcode572
chrome/browser/extensions/extensions_service.cc:572: DCHECK(!profile_);  //
profile should have told us it's going away.
nit: profile -> Profile