Split testing_pref_service.h/.cc into two parts, generic vs. Chrome-specific.

Split testing_pref_service.h/.cc into two parts, generic vs. Chrome-specific.

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

[Jói, 2013-02-12 13:06:34]

This is part of moving the non-Chrome-specific Prefs tests to base/prefs/.  My
plan is as follows:

a) Split testing_pref_service.h/.cc (current change).
b) Split pref_service_unittest.cc into generic vs. Chrome-specific parts.
c) Move remaining generic Prefs tests to base/prefs.

That will be the end of componentizing Prefs; next after that I'll work on the
various TODOs remaining related to the registration cleanup we're working
towards, and removing the PrefServiceBase interface.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-02-12 13:53:32]

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_utils_browsertest.cc:39: #include
"chrome/test/base/testing_pref_service_syncable.h"
needed?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/cros_settings_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/cros_settings_unittest.cc:21: #include
"chrome/test/base/testing_pref_service_syncable.h"
needed?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc
(right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:22:
#include "chrome/test/base/testing_pref_service_syncable.h"
This should remain testing_pref_service.h I guess?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/policy/auto_en...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/policy/auto_en...
chrome/browser/policy/auto_enrollment_client_unittest.cc:15: #include
"chrome/test/base/testing_pref_service_syncable.h"
should remain testing_pref_service.h?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/profiles/off_t...
File chrome/browser/profiles/off_the_record_profile_impl_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/profiles/off_t...
chrome/browser/profiles/off_the_record_profile_impl_unittest.cc:16: #include
"chrome/test/base/testing_pref_service_syncable.h"
would testing_pref_service.h suffice?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/profiles/profi...
File chrome/browser/profiles/profile_manager_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/profiles/profi...
chrome/browser/profiles/profile_manager_unittest.cc:34: #include
"chrome/test/base/testing_pref_service_syncable.h"
required?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/ui/chrome_sele...
File chrome/browser/ui/chrome_select_file_policy_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/ui/chrome_sele...
chrome/browser/ui/chrome_select_file_policy_unittest.cc:18: #include
"chrome/test/base/testing_pref_service_syncable.h"
testing_pref_service.h suffices?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/web_resource/p...
File chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc
(right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/web_resource/p...
chrome/browser/web_resource/promo_resource_service_mobile_ntp_unittest.cc:19:
#include "chrome/test/base/testing_pref_service_syncable.h"
testing_pref_service.h suffices?

https://codereview.chromium.org/12223091/diff/1/chrome/browser/web_resource/p...
File chrome/browser/web_resource/promo_resource_service_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/web_resource/p...
chrome/browser/web_resource/promo_resource_service_unittest.cc:24: #include
"chrome/test/base/testing_pref_service_syncable.h"
testing_pref_service.h suffices?

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
File chrome/test/base/testing_pref_service_syncable.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.cc:32:
static_cast<PrefRegistrySyncable*>(pref_registry),
Hm, so this hack is here to stay? I guess the only real chance to clean this up
is getting rid of PrefServiceSyncable?

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.cc:61: local_state_.registry());
nit: fits previous line.

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
File chrome/test/base/testing_pref_service_syncable.h (right):

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.h:24: // e.g.
PrefServiceBuilder).
fits the previous line?

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.h:28:
DISALLOW_COPY_AND_ASSIGN(TestingPrefServiceSyncable);
#include "base/basictypes.h"

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_prof...
File chrome/test/base/testing_profile_manager.h (right):

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_prof...
chrome/test/base/testing_profile_manager.h:14: #include
"chrome/test/base/testing_pref_service_syncable.h"
needed?

[Jói, 2013-02-12 14:35:50]

I think all the #includes of testing_pref_service_syncable.h that you tagged are
required; I added them all either due to compiler errors or because of usage of
TestingPrefServiceSyncable or ScopedTestingLocalState.  I could move the latter
class to its own separate file and switch those includes, would make things a
bit cleaner.

See below for other responses (plus a bunch of ScopedTestingLocalState
responses; I stopped after a few).

Cheers,
Jói

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/login...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/login...
chrome/browser/chromeos/login/login_utils_browsertest.cc:39: #include
"chrome/test/base/testing_pref_service_syncable.h"
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> needed?

For ScopedTestingLocalState.

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/cros_settings_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/cros_settings_unittest.cc:21: #include
"chrome/test/base/testing_pref_service_syncable.h"
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> needed?

For ScopedTestingLocalState.

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
File chrome/browser/chromeos/settings/device_settings_provider_unittest.cc
(right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/chromeos/setti...
chrome/browser/chromeos/settings/device_settings_provider_unittest.cc:22:
#include "chrome/test/base/testing_pref_service_syncable.h"
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> This should remain testing_pref_service.h I guess?

This is for ScopedTestingLocalState.

https://codereview.chromium.org/12223091/diff/1/chrome/browser/policy/auto_en...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/browser/policy/auto_en...
chrome/browser/policy/auto_enrollment_client_unittest.cc:15: #include
"chrome/test/base/testing_pref_service_syncable.h"
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> should remain testing_pref_service.h?

ScopedTestingLocalState.

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
File chrome/test/base/testing_pref_service_syncable.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.cc:32:
static_cast<PrefRegistrySyncable*>(pref_registry),
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> Hm, so this hack is here to stay? I guess the only real chance to clean this
up
> is getting rid of PrefServiceSyncable?

Which hack do you mean, the cast to PrefRegistrySyncable? That's just to be able
to reuse TestingPrefServiceBase while at the same time PrefServiceSyncable
requires a PrefRegistrySyncable rather than the generic PrefRegistry type.

I don't think it's possible to get rid of the cast, it needs to be either here
or in the PrefServiceSyncable constructor, and moving it to the constructor
would mean less type safety.

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.cc:61: local_state_.registry());
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> nit: fits previous line.

Done.

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
File chrome/test/base/testing_pref_service_syncable.h (right):

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.h:24: // e.g.
PrefServiceBuilder).
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> fits the previous line?

Done.

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.h:28:
DISALLOW_COPY_AND_ASSIGN(TestingPrefServiceSyncable);
On 2013/02/12 13:53:32, Mattias Nissler wrote:
> #include "base/basictypes.h"

Done.

[Mattias Nissler (ping if slow), 2013-02-12 14:42:57]

Doh, I had wrongly assumed ScopedTestingLocalState was declared in
testing_pref_service.h, which it obviously isn't. I guess it actually makes
sense to move ScopedTestingLocalState to its own header, because it's got
nothing to do with they syncable business.

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
File chrome/test/base/testing_pref_service_syncable.cc (right):

https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
chrome/test/base/testing_pref_service_syncable.cc:32:
static_cast<PrefRegistrySyncable*>(pref_registry),
On 2013/02/12 14:35:51, Jói wrote:
> On 2013/02/12 13:53:32, Mattias Nissler wrote:
> > Hm, so this hack is here to stay? I guess the only real chance to clean this
> up
> > is getting rid of PrefServiceSyncable?
> 
> Which hack do you mean, the cast to PrefRegistrySyncable? That's just to be
able
> to reuse TestingPrefServiceBase while at the same time PrefServiceSyncable
> requires a PrefRegistrySyncable rather than the generic PrefRegistry type.
> 
> I don't think it's possible to get rid of the cast, it needs to be either here
> or in the PrefServiceSyncable constructor, and moving it to the constructor
> would mean less type safety.

Yes, I mean the cast. Thinking more about it, there is one option: Add the
registry as a template parameter. WDYT?

[Jói, 2013-02-12 14:46:22]

Good idea, will try making the registry a template parameter.

Will also move ScopedTestingLocalState to its own header.

Cheers,
Jói


On Tue, Feb 12, 2013 at 2:42 PM,  <mnissler@chromium.org> wrote:
> Doh, I had wrongly assumed ScopedTestingLocalState was declared in
> testing_pref_service.h, which it obviously isn't. I guess it actually makes
> sense to move ScopedTestingLocalState to its own header, because it's got
> nothing to do with they syncable business.
>
>
>
>
https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
> File chrome/test/base/testing_pref_service_syncable.cc (right):
>
>
https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
> chrome/test/base/testing_pref_service_syncable.cc:32:
> static_cast<PrefRegistrySyncable*>(pref_registry),
> On 2013/02/12 14:35:51, Jói wrote:
>>
>> On 2013/02/12 13:53:32, Mattias Nissler wrote:
>> > Hm, so this hack is here to stay? I guess the only real chance to
>
> clean this
>>
>> up
>> > is getting rid of PrefServiceSyncable?
>
>
>> Which hack do you mean, the cast to PrefRegistrySyncable? That's just
>
> to be able
>>
>> to reuse TestingPrefServiceBase while at the same time
>
> PrefServiceSyncable
>>
>> requires a PrefRegistrySyncable rather than the generic PrefRegistry
>
> type.
>
>> I don't think it's possible to get rid of the cast, it needs to be
>
> either here
>>
>> or in the PrefServiceSyncable constructor, and moving it to the
>
> constructor
>>
>> would mean less type safety.
>
>
> Yes, I mean the cast. Thinking more about it, there is one option: Add
> the registry as a template parameter. WDYT?
>
> https://codereview.chromium.org/12223091/

[Jói, 2013-02-12 16:38:24]

> Good idea, will try making the registry a template parameter.
> 
> Will also move ScopedTestingLocalState to its own header.

PTAL, I've done both of the above.

Any includes of testing_profile_keyed_service_syncable.h at this point are
either because there is an occurrence of the string ProfileKeyedServiceSyncable,
or because the compiler complained that it needed the definition of the class.

Cheers,
Jói



On 2013/02/12 14:46:22, Jói wrote:
> Good idea, will try making the registry a template parameter.
> 
> Will also move ScopedTestingLocalState to its own header.
> 
> Cheers,
> Jói
> 
> 
> On Tue, Feb 12, 2013 at 2:42 PM,  <mailto:mnissler@chromium.org> wrote:
> > Doh, I had wrongly assumed ScopedTestingLocalState was declared in
> > testing_pref_service.h, which it obviously isn't. I guess it actually makes
> > sense to move ScopedTestingLocalState to its own header, because it's got
> > nothing to do with they syncable business.
> >
> >
> >
> >
>
https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
> > File chrome/test/base/testing_pref_service_syncable.cc (right):
> >
> >
>
https://codereview.chromium.org/12223091/diff/1/chrome/test/base/testing_pref...
> > chrome/test/base/testing_pref_service_syncable.cc:32:
> > static_cast<PrefRegistrySyncable*>(pref_registry),
> > On 2013/02/12 14:35:51, Jói wrote:
> >>
> >> On 2013/02/12 13:53:32, Mattias Nissler wrote:
> >> > Hm, so this hack is here to stay? I guess the only real chance to
> >
> > clean this
> >>
> >> up
> >> > is getting rid of PrefServiceSyncable?
> >
> >
> >> Which hack do you mean, the cast to PrefRegistrySyncable? That's just
> >
> > to be able
> >>
> >> to reuse TestingPrefServiceBase while at the same time
> >
> > PrefServiceSyncable
> >>
> >> requires a PrefRegistrySyncable rather than the generic PrefRegistry
> >
> > type.
> >
> >> I don't think it's possible to get rid of the cast, it needs to be
> >
> > either here
> >>
> >> or in the PrefServiceSyncable constructor, and moving it to the
> >
> > constructor
> >>
> >> would mean less type safety.
> >
> >
> > Yes, I mean the cast. Thinking more about it, there is one option: Add
> > the registry as a template parameter. WDYT?
> >
> > https://codereview.chromium.org/12223091/

[Jói, 2013-02-12 17:33:09]

TBR=ben@chromium.org for #include statement updates throughout the code.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-02-12 17:34:47]

LGTM with a nit/suggestion.

https://codereview.chromium.org/12223091/diff/11001/chrome/test/base/testing_...
File chrome/test/base/testing_pref_service.h (right):

https://codereview.chromium.org/12223091/diff/11001/chrome/test/base/testing_...
chrome/test/base/testing_pref_service.h:23: template <class SuperPrefService,
class ConstructionPrefRegistry=PrefRegistry>
nit: Other code seems to put spaces around =

After seeing the rest of the code, it seems that the default only avoids the
second parameter in very few places. How about just dropping the default and
making this explicit everywhere?

[commit-bot: I haz the power, 2013-02-12 19:33:13]

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

[Jói, 2013-02-12 19:45:34]

Thanks Mattias. Forgot to respond with the comment below before pulling the CQ
trigger.

Cheers,
Jói

https://codereview.chromium.org/12223091/diff/11001/chrome/test/base/testing_...
File chrome/test/base/testing_pref_service.h (right):

https://codereview.chromium.org/12223091/diff/11001/chrome/test/base/testing_...
chrome/test/base/testing_pref_service.h:23: template <class SuperPrefService,
class ConstructionPrefRegistry=PrefRegistry>
On 2013/02/12 17:34:47, Mattias Nissler wrote:
> nit: Other code seems to put spaces around =
> 
> After seeing the rest of the code, it seems that the default only avoids the
> second parameter in very few places. How about just dropping the default and
> making this explicit everywhere?

Done.

[commit-bot: I haz the power, 2013-02-12 21:45:16]

Change committed as 182026

[Ben Goodger (Google), 2013-02-13 22:23:18]

lgtm