prefs / sync: rewrite ProfileSyncServicePreferenceTest to rely on sync/api instead of c/b/s/glue.

prefs / sync: rewrite ProfileSyncServicePreferenceTest to rely on sync/api instead of c/b/s/glue.

There's no need to use GenericChangeProcessor for a test like this.

TBR=mnissler@chromium.org
BUG=339726
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267009

[Mattias Nissler (ping if slow), 2014-04-25 09:10:33]

I think zea@ originally wrote this code, so he should review. I'm happy to
rubber-stamp once a reviewer who understands the sync stuff approves.

[tim (not reviewing), 2014-04-25 16:32:55]

Ah, thanks. In that case, probably only zea@ is required to review (rlarocque,
feel free to skip)

[Nicolas Zea, 2014-04-25 20:43:18]

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:5: #ifndef
CHROME_BROWSER_PREFS_PREF_MODEL_ASSOCIATOR_H_
Should we go ahead and rename this prefs_syncable_service?

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
File chrome/browser/prefs/prefs_syncable_service_unittest.cc (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:75:
example_url0_("http://example.com/0"),
make all these const char[]'s defined in an anon namespace?

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:85: profile_ =
profile_manager_.CreateTestingProfile(
Do you need a testing profile (and the profile manager)?
TestingPrefServiceSyncable doesn't rely on one as far as I can tell, nor does
the pref syncable service.

Could you not just instantiate your own TestingPrefServiceSyncable?

Maybe see
https://codereview.chromium.org/195873020/diff/200001/components/sync_driver/...
for reference

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:134: void
AddPrefToDataList(const std::string& name,
nit: AddPrefToRemoteDataList is a bit clearer I think

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:173: const base::Value*
FindValue(const std::string& name,
Is this transferring ownereship? Return scoped_ptr perhaps?

[Mattias Nissler (ping if slow), 2014-04-28 11:35:41]

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:5: #ifndef
CHROME_BROWSER_PREFS_PREF_MODEL_ASSOCIATOR_H_
On 2014/04/25 20:43:18, Nicolas Zea wrote:
> Should we go ahead and rename this prefs_syncable_service?

That would create a great deal of confusion given the existing
PrefServiceSyncable.

[tim (not reviewing), 2014-04-28 18:28:51]

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:5: #ifndef
CHROME_BROWSER_PREFS_PREF_MODEL_ASSOCIATOR_H_
On 2014/04/25 20:43:18, Nicolas Zea wrote:
> Should we go ahead and rename this prefs_syncable_service?

Hmm, seems like it. Wasn't sure why it was named this way actually.  I just
noticed pref_model_associator_unittest exists, although it doesn't test any of
the sync stuff.  Mind if I do a follow up to rename+merge these two things?

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
File chrome/browser/prefs/prefs_syncable_service_unittest.cc (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:75:
example_url0_("http://example.com/0"),
On 2014/04/25 20:43:18, Nicolas Zea wrote:
> make all these const char[]'s defined in an anon namespace?

Done.

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:85: profile_ =
profile_manager_.CreateTestingProfile(
On 2014/04/25 20:43:18, Nicolas Zea wrote:
> Do you need a testing profile (and the profile manager)?
> TestingPrefServiceSyncable doesn't rely on one as far as I can tell, nor does
> the pref syncable service.
> 
> Could you not just instantiate your own TestingPrefServiceSyncable?
> 
> Maybe see
>
https://codereview.chromium.org/195873020/diff/200001/components/sync_driver/...
> for reference

Maybe although that's a lot of boilerplate to write in place of
CreateTestingProfile and GetTestingPrefService, which seem to exist for this
purpose... I'd also have to make sure to set up both managed and user prefs for
this test. I'm not convinced it's worth it, but if you feel strongly.   I did
manage to simplify the CreateTestingProfile call.

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:134: void
AddPrefToDataList(const std::string& name,
On 2014/04/25 20:43:18, Nicolas Zea wrote:
> nit: AddPrefToRemoteDataList is a bit clearer I think

Good call on 'Remote', went with AddToRemoteDataList which I think is still
clear but is a bit more compact :)

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:173: const base::Value*
FindValue(const std::string& name,
On 2014/04/25 20:43:18, Nicolas Zea wrote:
> Is this transferring ownereship? Return scoped_ptr perhaps?

Done.

[tim (not reviewing), 2014-04-28 18:29:51]

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.h (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.h:5: #ifndef
CHROME_BROWSER_PREFS_PREF_MODEL_ASSOCIATOR_H_
On 2014/04/28 18:28:51, timsteele wrote:
> On 2014/04/25 20:43:18, Nicolas Zea wrote:
> > Should we go ahead and rename this prefs_syncable_service?
> 
> Hmm, seems like it. Wasn't sure why it was named this way actually.  I just
> noticed pref_model_associator_unittest exists, although it doesn't test any of
> the sync stuff.  Mind if I do a follow up to rename+merge these two things?

Drafted this before I saw mnissler's mail... As a non-prefs expert I defer to
his judgement on this one.

[Nicolas Zea, 2014-04-28 18:38:54]

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
File chrome/browser/prefs/prefs_syncable_service_unittest.cc (right):

https://codereview.chromium.org/258753002/diff/1/chrome/browser/prefs/prefs_s...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:85: profile_ =
profile_manager_.CreateTestingProfile(
On 2014/04/28 18:28:51, timsteele wrote:
> On 2014/04/25 20:43:18, Nicolas Zea wrote:
> > Do you need a testing profile (and the profile manager)?
> > TestingPrefServiceSyncable doesn't rely on one as far as I can tell, nor
does
> > the pref syncable service.
> > 
> > Could you not just instantiate your own TestingPrefServiceSyncable?
> > 
> > Maybe see
> >
>
https://codereview.chromium.org/195873020/diff/200001/components/sync_driver/...
> > for reference
> 
> Maybe although that's a lot of boilerplate to write in place of
> CreateTestingProfile and GetTestingPrefService, which seem to exist for this
> purpose... I'd also have to make sure to set up both managed and user prefs
for
> this test. I'm not convinced it's worth it, but if you feel strongly.   I did
> manage to simplify the CreateTestingProfile call. 

I just realized the reason I had that boilerplate was because the code lived in
a component, and testing_pref_service_syncable.h lives in chrome/test. You
should be able to use it directly I think, without needing any boilerplate.

[tim (not reviewing), 2014-04-28 20:01:24]

Removed profile stuff to use TestPrefServiceSyncable directly.

[Nicolas Zea, 2014-04-28 20:10:28]

LGTM

https://codereview.chromium.org/258753002/diff/60001/chrome/browser/prefs/pre...
File chrome/browser/prefs/prefs_syncable_service_unittest.cc (right):

https://codereview.chromium.org/258753002/diff/60001/chrome/browser/prefs/pre...
chrome/browser/prefs/prefs_syncable_service_unittest.cc:201:
scoped_ptr<TestingPrefServiceSyncable> prefs_;
nit: does this need to be a scoped ptr? Can you just instantiate it directly?

[tim (not reviewing), 2014-04-28 20:23:19]

On 2014/04/28 20:10:28, Nicolas Zea wrote:
> LGTM
> 
>
https://codereview.chromium.org/258753002/diff/60001/chrome/browser/prefs/pre...
> File chrome/browser/prefs/prefs_syncable_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/258753002/diff/60001/chrome/browser/prefs/pre...
> chrome/browser/prefs/prefs_syncable_service_unittest.cc:201:
> scoped_ptr<TestingPrefServiceSyncable> prefs_;
> nit: does this need to be a scoped ptr? Can you just instantiate it directly?

Done.  

@mnissler, ready when you are.

[tim (not reviewing), 2014-04-29 18:47:17]

moving mnissler to TBR under the assumption he still was planning to
rubber-stamp following zea's review.  Happy to make follow up changes if
necessary.

[tim (not reviewing), 2014-04-29 18:47:33]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-04-29 18:49:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/258753002/80001

[commit-bot: I haz the power, 2014-04-30 00:39:42]

Change committed as 267009