Fix prefs registration in SyncPrefs.

Fix prefs registration in SyncPrefs.

All preferences should be registered _before_ a PrefService is created,
so reading other prefs during registration is a no-no.

The previous code dynamically set a default for the
kSyncKeepEverythingSynced preference and for each data type preference
dynamically based on whether or not sync setup is complete, but during
sync setup each of these would be explicitly set anyway, overriding
the default.

The one complexity is that there is code in
ProfileSyncService::TrySyncDatatypePrefRecovery that attempts to
recover from an old bug, and it depends on checking whether the value
is default. We remove one of the checks it used, which was redundant
(the needed check is just whether the pref is still at its default
value, regardless of that default value).

TBR=ben@chromium.org
BUG=155525
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184800

[Jói, 2013-02-22 15:35:05]

nick: chrome/browser/sync
mnissler: entire change

NOTE: I'm not 100% sure this is correct. Nick, would like your opinion on it.
From my reading of the code, the preferences for which data types to sync get
set explicitly in every case by the time sync is turned on, so their default
value doesn't really matter that much.

If the above is incorrect, we'll have to figure something else out. The fact
that ProfileSyncService::TrySyncDatatypePrefRecovery relies on checking whether
the prefs have their default values complicate things a little, in that we can't
just explicitly set them to whatever the desired value might be post
registration.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-02-22 15:44:32]

LGTM in general, I haven't checked the subtleties of the sync default changes
though, deferring to Nick on that.

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/prefs/brows...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/prefs/brows...
chrome/browser/prefs/browser_prefs.cc:259: void
RegisterUserPrefs(PrefRegistrySyncable* registry) {
w00t!

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:392:
Profile::RegisterUserPrefs(pref_registry_);
Suggestion: move this call into chrome::RegisterUserPrefs as well?

[Jói, 2013-02-22 16:08:07]

Thanks Mattias.  Will await Nick's feedback for the rest.

Cheers,
Jói

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/prefs/brows...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/prefs/brows...
chrome/browser/prefs/browser_prefs.cc:259: void
RegisterUserPrefs(PrefRegistrySyncable* registry) {
On 2013/02/22 15:44:32, Mattias Nissler wrote:
> w00t!

Indeed :)

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/12315053/diff/3001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:392:
Profile::RegisterUserPrefs(pref_registry_);
On 2013/02/22 15:44:32, Mattias Nissler wrote:
> Suggestion: move this call into chrome::RegisterUserPrefs as well?

Done.

[ncarter (slow), 2013-02-22 17:38:17]

rlarocque, timsteele, or zea would be a better reviewer than me. Routing to
Richard.

[rlarocque, 2013-02-22 21:50:08]

Unfortunately, I don't know this area well enough to review these changes.

I think Nicolas would be the best reviewer, but he won't be able to review this
until Monday at the earliest.  

I'll add Tim to the reviewers list, too.  Maybe he'll be able to get you some
useful feedback a bit earlier.

[Jói, 2013-02-22 21:52:23]

Thanks.  If this needs to wait until Monday that's fine.

Cheers,
Jói


On Fri, Feb 22, 2013 at 9:50 PM,  <rlarocque@chromium.org> wrote:
> Unfortunately, I don't know this area well enough to review these changes.
>
> I think Nicolas would be the best reviewer, but he won't be able to review
> this
> until Monday at the earliest.
>
> I'll add Tim to the reviewers list, too.  Maybe he'll be able to get you
> some
> useful feedback a bit earlier.
>
> https://codereview.chromium.org/12315053/

[Nicolas Zea, 2013-02-25 23:58:37]

sync LGTM

[Jói, 2013-02-26 14:48:45]

Thanks guys.

FYI, the latest patch set has a small change to browser_prefs.cc and callers of
chrome::RegisterUserPrefs, in that the PrefService parameter is now not going
away.  This is because I'm landing this change before
https://codereview.chromium.org/12328054/ (Sreeram is taking it over), which
previously was before this one in the pipeline.  I updated the change
description to delete the paragraph saying I was removing that parameter.

Cheers,
Jói

[commit-bot: I haz the power, 2013-02-26 22:18:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12315053/10012

[commit-bot: I haz the power, 2013-02-26 22:18:07]

Presubmit check for 12315053-10012 failed and returned exit status 1.

INFO:root:Found 10 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/sync/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
    chrome/browser/profiles/profile_impl.cc
    chrome/browser/extensions/extension_service_unittest.cc
    chrome/test/base/testing_profile.cc

Presubmit checks took 1.5s to calculate.

[Jói, 2013-02-26 22:20:02]

TBR=ben@chromium.org for Prefs-related updates across codebase, already reviewed
by mnissler@ for Prefs OWNERS.

[commit-bot: I haz the power, 2013-02-26 22:23:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/12315053/10012

[commit-bot: I haz the power, 2013-02-27 00:42:01]

Change committed as 184800