Adjust preference sync code to only sync user modifiable preferences.

Adjust preference sync code to only sync user modifiable preferences.

Switch to the new preference value source checkers in Preference. While at it,
add a unit test and better test infrastructure for controlling preference
values in tests. Convert existing unit tests where appropriate.

BUG=48952
TEST=ProfileSyncServicePreferenceTest.ManagedPreferences

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

[Mattias Nissler, 2010-07-19 11:57:18]

Please review. Note that I'm not positive whether the changes to
TestingProfile::GetPrefs() prove viable, I'm waiting for the trybots to give an
answer to that.

[Pam (also PM for reviews), 2010-07-19 13:19:05]

You can point this at bug 48952 for the sake of having something there. Having a
bug referenced helps QA know a little more about what a patch is doing and how
it might be manually tested if necessary (or that it doesn't need to be).

http://codereview.chromium.org/3051001/diff/1/4
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/3051001/diff/1/4#newcode126
chrome/browser/pref_value_store.cc:126: // No need to look in PrefStores with
lower priority than the one we want.
This comment is now wrong.

http://codereview.chromium.org/3051001/diff/1/5
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/3051001/diff/1/5#newcode124
chrome/browser/pref_value_store.h:124: PrefStoreType
EffectivePrefStoreForPref(const wchar_t* name);
Since the PrefService uses "control" to describe this situation, how about
ControllingPrefStoreForPref()?

http://codereview.chromium.org/3051001/diff/1/6
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/3051001/diff/1/6#newcode145
chrome/browser/sync/glue/preference_change_processor.cc:145: // Don't try to
overwrite preferences controlled by policy.
"controlled by policy" -> "not controllable by the user"

http://codereview.chromium.org/3051001/diff/1/7
File chrome/browser/sync/glue/preference_model_associator.cc (right):

http://codereview.chromium.org/3051001/diff/1/7#newcode107
chrome/browser/sync/glue/preference_model_associator.cc:107: } else if
(pref->IsUserControlled()) {
Some internal comments would help here. What does the test for
InitByClientTagLookup do above, and why does the earlier test look at
UserModifiable while this one looks at UserControlled?

http://codereview.chromium.org/3051001/diff/1/13
File chrome/test/testing_pref_service.cc (right):

http://codereview.chromium.org/3051001/diff/1/13#newcode12
chrome/test/testing_pref_service.cc:12: managed_prefs_ = new DummyPrefStore(),
Does this not fit on the line above? If it does, it might be clearer to move it
up, and line up the ones below with it.

http://codereview.chromium.org/3051001/diff/1/13#newcode62
chrome/test/testing_pref_service.cc:62: if (removed_value)
No need to check this; 'delete NULL' is safe.

http://codereview.chromium.org/3051001/diff/1/14
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/3051001/diff/1/14#newcode12
chrome/test/testing_pref_service.h:12: // (managed, extension, user)
conveniently.
Nice. Bonus points for using this in all the other unit tests (Pref{Value|}Store
and PrefService, plus all the ones that just want a local set of prefs) too.

http://codereview.chromium.org/3051001/diff/1/14#newcode16
chrome/test/testing_pref_service.h:16: explicit TestingPrefService();
No need for 'explicit' -- that's only when you have a constructor with exactly
one parameter.

http://codereview.chromium.org/3051001/diff/1/14#newcode19
chrome/test/testing_pref_service.h:19: // preferences is not defined at the
managed layer.
preferences -> preference

http://codereview.chromium.org/3051001/diff/1/14#newcode38
chrome/test/testing_pref_service.h:38: static const Value* GetPref(PrefStore*
pref_store, const wchar_t* path);
Wow, I can't recall the last time I saw a private static method. No problem,
though.

[Mattias Nissler, 2010-07-19 16:45:11]

I've uploaded a new version with nits fixed, I'll take care of converting tests
tomorrow.

http://codereview.chromium.org/3051001/diff/1/4
File chrome/browser/pref_value_store.cc (right):

http://codereview.chromium.org/3051001/diff/1/4#newcode126
chrome/browser/pref_value_store.cc:126: // No need to look in PrefStores with
lower priority than the one we want.
On 2010/07/19 13:19:06, Pam wrote:
> This comment is now wrong.

Done.

http://codereview.chromium.org/3051001/diff/1/6
File chrome/browser/sync/glue/preference_change_processor.cc (right):

http://codereview.chromium.org/3051001/diff/1/6#newcode145
chrome/browser/sync/glue/preference_change_processor.cc:145: // Don't try to
overwrite preferences controlled by policy.
On 2010/07/19 13:19:06, Pam wrote:
> "controlled by policy" -> "not controllable by the user"

Done.

http://codereview.chromium.org/3051001/diff/1/7
File chrome/browser/sync/glue/preference_model_associator.cc (right):

http://codereview.chromium.org/3051001/diff/1/7#newcode107
chrome/browser/sync/glue/preference_model_associator.cc:107: } else if
(pref->IsUserControlled()) {
On 2010/07/19 13:19:06, Pam wrote:
> Some internal comments would help here. What does the test for
> InitByClientTagLookup do above, and why does the earlier test look at
> UserModifiable while this one looks at UserControlled?

Done.

http://codereview.chromium.org/3051001/diff/1/13
File chrome/test/testing_pref_service.cc (right):

http://codereview.chromium.org/3051001/diff/1/13#newcode12
chrome/test/testing_pref_service.cc:12: managed_prefs_ = new DummyPrefStore(),
On 2010/07/19 13:19:06, Pam wrote:
> Does this not fit on the line above? If it does, it might be clearer to move
it
> up, and line up the ones below with it.

Tried it, and it ends up looking ugly due to the orphaned closing brace in line
17. I'd rather keep it this way, if that's ok with you.

http://codereview.chromium.org/3051001/diff/1/13#newcode62
chrome/test/testing_pref_service.cc:62: if (removed_value)
On 2010/07/19 13:19:06, Pam wrote:
> No need to check this; 'delete NULL' is safe.

It's so much easier to just pass NULL. And it's even documented. Mental note to
myself: Always read comments entirely before writing code...

http://codereview.chromium.org/3051001/diff/1/14
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/3051001/diff/1/14#newcode12
chrome/test/testing_pref_service.h:12: // (managed, extension, user)
conveniently.
On 2010/07/19 13:19:06, Pam wrote:
> Nice. Bonus points for using this in all the other unit tests
(Pref{Value|}Store
> and PrefService, plus all the ones that just want a local set of prefs) too.

I'll go and get the bonus points. Tomorrow :)

http://codereview.chromium.org/3051001/diff/1/14#newcode16
chrome/test/testing_pref_service.h:16: explicit TestingPrefService();
On 2010/07/19 13:19:06, Pam wrote:
> No need for 'explicit' -- that's only when you have a constructor with exactly
> one parameter.

Done.

http://codereview.chromium.org/3051001/diff/1/14#newcode19
chrome/test/testing_pref_service.h:19: // preferences is not defined at the
managed layer.
On 2010/07/19 13:19:06, Pam wrote:
> preferences -> preference

Done.

[skrul, 2010-07-19 19:21:18]

Sync parts LGTM, thanks for the unit test!

[Mattias Nissler, 2010-07-20 09:48:15]

Pam, would you take another look? Thanks.

[Pam (also PM for reviews), 2010-07-20 10:46:29]

LGTM once the below comments are addressed and trybots pass. No need for
re-review unless you're uncertain of anything.

- Pam

http://codereview.chromium.org/3051001/diff/11001/12001
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/3051001/diff/11001/12001#newcode164
chrome/browser/dom_ui/new_tab_ui_uitest.cc:164: }
Please restore the trailing CR. Some compilers complain without them.

http://codereview.chromium.org/3051001/diff/11001/12002
File chrome/browser/dom_ui/shown_sections_handler_unittest.cc (left):

http://codereview.chromium.org/3051001/diff/11001/12002#oldcode42
chrome/browser/dom_ui/shown_sections_handler_unittest.cc:42:
scoped_ptr<PrefService> pref(PrefService::CreateUserPrefService(user_prefs));
Does anything use CreateUserPrefService anymore, or could it be removed?

http://codereview.chromium.org/3051001/diff/11001/12011
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/3051001/diff/11001/12011#newcode124
chrome/browser/pref_value_store.h:124: PrefStoreType
EffectivePrefStoreForPref(const wchar_t* name);
Did you miss my suggestion of renaming this "ControllingPrefStore", to be
clearer as well as matching what the PrefService uses? It's not required; I just
want to be sure you noticed.

http://codereview.chromium.org/3051001/diff/11001/12020
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/3051001/diff/11001/12020#newcode38
chrome/test/testing_pref_service.h:38: const Value* GetPref(PrefStore*
pref_store, const wchar_t* path);
Were they not fine as static? No objection either way.

[Mattias Nissler, 2010-07-20 13:05:40]

Once the trybots give their OK (in particular the memory leak is gone), I'm
going to commit this. Speak up if you have objections :)

http://codereview.chromium.org/3051001/diff/11001/12001
File chrome/browser/dom_ui/new_tab_ui_uitest.cc (right):

http://codereview.chromium.org/3051001/diff/11001/12001#newcode164
chrome/browser/dom_ui/new_tab_ui_uitest.cc:164: }
On 2010/07/20 10:46:29, Pam wrote:
> Please restore the trailing CR. Some compilers complain without them.

Note there still _is_ a trailing CR, but no trailing blank line (which amounts
to two CRs). pkasting actually advised me to do what I did. Technically, it's
not even me, but google.vim :)

http://codereview.chromium.org/3051001/diff/11001/12002
File chrome/browser/dom_ui/shown_sections_handler_unittest.cc (left):

http://codereview.chromium.org/3051001/diff/11001/12002#oldcode42
chrome/browser/dom_ui/shown_sections_handler_unittest.cc:42:
scoped_ptr<PrefService> pref(PrefService::CreateUserPrefService(user_prefs));
On 2010/07/20 10:46:29, Pam wrote:
> Does anything use CreateUserPrefService anymore, or could it be removed?

There are still tests that rely on file-backed PrefServices (e.g. for checking
results from a different process), so I kept it.

http://codereview.chromium.org/3051001/diff/11001/12011
File chrome/browser/pref_value_store.h (right):

http://codereview.chromium.org/3051001/diff/11001/12011#newcode124
chrome/browser/pref_value_store.h:124: PrefStoreType
EffectivePrefStoreForPref(const wchar_t* name);
On 2010/07/20 10:46:29, Pam wrote:
> Did you miss my suggestion of renaming this "ControllingPrefStore", to be
> clearer as well as matching what the PrefService uses? It's not required; I
just
> want to be sure you noticed.

Yes, I missed that comment, sorry. I agree that it's clearer and changed it.

http://codereview.chromium.org/3051001/diff/11001/12020
File chrome/test/testing_pref_service.h (right):

http://codereview.chromium.org/3051001/diff/11001/12020#newcode38
chrome/test/testing_pref_service.h:38: const Value* GetPref(PrefStore*
pref_store, const wchar_t* path);
On 2010/07/20 10:46:29, Pam wrote:
> Were they not fine as static? No objection either way.

They now also fire observers, so they can't be static.