Introduce PrefRegistrySyncable, simplifying PrefServiceSyncable.

Introduce PrefRegistrySyncable, simplifying PrefServiceSyncable.

Minimize usage of PrefServiceSyncable. It is mostly an implementation detail of ProfileImpl and the sync integration.

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

[Jói, 2013-01-31 15:01:27]

Hi Mattias,

This is a preview of just the core classes that change when introducing
PrefRegistrySyncable, to see if you agree with the approach before I do more
work.

The rest of the change would:
a) Make PrefServiceSyncable an implementation detail of Profile;
b) Use PrefRegistrySyncable for registrations, where PrefServiceSyncable is
currently used;
c) Update other usage to match the new interfaces.

In follow-up changes, I plan to address the remaining TODOs needed before I can
move all the non-Chrome-specific Prefs code to base/prefs/ (e.g. split
testing_pref_service.h and .cc in two to leave the syncable parts on the
src/chrome side).

In a second batch of follow-ups, I want to clean up the after-the-fact
registrations we know of in production code.

Cheers,
Jói

[Mattias Nissler (ping if slow), 2013-01-31 15:30:19]

Preview and plan for future work looks good, thanks!

https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_registry_syncable.cc (right):

https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_registry_syncable.cc:190: PrefSyncStatus sync_status)
{
Some thoughts here: We were thinking of splitting the syncable stuff away from
PrefService entirely and drive PrefModelAssociator via pref change notifications
like everything else. When we do that, we might consider getting rid of
PrefServiceSyncable as well, keep the name-to-syncable-flag-map here in
PrefRegistrySyncable and pass the PrefRegistrySyncable reference to
PrefModelAssociator at construction time.

I haven't thought through all the details, but if this works out, it would
further modularize and simplify things.

https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
File chrome/browser/prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_registry_syncable.h:24: typedef
base::Callback<void(const char* path)> SyncableRegistrationCallback;
Callback needs #include or forward-declaration

https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
chrome/browser/prefs/pref_registry_syncable.h:95: SyncableRegistrationCallback
callback_;
DISALLOW_COPY_AND_ASSIGN

[Jói, 2013-01-31 15:38:10]

Thanks Mattias.  After sending you the patch I started thinking along
similar lines, about completely getting rid of PrefServiceSyncable.
I'll take a better look to see if that might actually be possible.

Cheers,
Jói


On Thu, Jan 31, 2013 at 3:30 PM,  <mnissler@chromium.org> wrote:
> Preview and plan for future work looks good, thanks!
>
>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> File chrome/browser/prefs/pref_registry_syncable.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> chrome/browser/prefs/pref_registry_syncable.cc:190: PrefSyncStatus
> sync_status) {
> Some thoughts here: We were thinking of splitting the syncable stuff
> away from PrefService entirely and drive PrefModelAssociator via pref
> change notifications like everything else. When we do that, we might
> consider getting rid of PrefServiceSyncable as well, keep the
> name-to-syncable-flag-map here in PrefRegistrySyncable and pass the
> PrefRegistrySyncable reference to PrefModelAssociator at construction
> time.
>
> I haven't thought through all the details, but if this works out, it
> would further modularize and simplify things.
>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> File chrome/browser/prefs/pref_registry_syncable.h (right):
>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> chrome/browser/prefs/pref_registry_syncable.h:24: typedef
> base::Callback<void(const char* path)> SyncableRegistrationCallback;
> Callback needs #include or forward-declaration
>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> chrome/browser/prefs/pref_registry_syncable.h:95:
> SyncableRegistrationCallback callback_;
> DISALLOW_COPY_AND_ASSIGN
>
> https://codereview.chromium.org/12079097/

[Jói, 2013-02-01 12:54:44]

I've taken a better look.  TL:DR; I'm planning to go ahead with the
current change as planned, but getting rid of PrefServiceSyncable
later is feasible.  More details below.

Getting rid of PrefServiceSyncable is doable, but orthogonal to the
current change, and if you/we decide to do it, it will be much easier
to think about and implement once this first change has been
committed.  Doing it all at once wouldn't simplify the current change,
I don't see much double-work if any.

PrefServiceSyncable has three main areas of responsibility remaining
once my current PrefRegistrySyncable change goes in.  Here's how they
might be moved to other places:
a) Tell PrefModelAssociator about registration changes. This could
move to PrefRegistrySyncable.
b) Notify observers of syncing changes, and provide access to the
SyncableService. This could move to PrefModelAssociator, which would
be exposed as e.g. Profile::GetPrefSyncService. One issue is that the
PrefModelAssociator interface is wider than what is currently exposed
via PrefSyncableService.
c) Know how to construct an incognito PrefService, and provide some
additional safety around that (see
PrefServiceSyncable::UpdateCommandLineStore). This could potentially
move to ProfileImpl.

Given the above, I'm ambivalent about whether getting rid of
PrefServiceSyncable is worth it.  If it is done, then more thought
should go into how PrefModelAssociator should be exposed to users, and
you move some knowledge that is specific to this subtype of a
PrefService away from a separate class and to ProfileImpl (c).

Cheers,
Jói



On Thu, Jan 31, 2013 at 3:37 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Thanks Mattias.  After sending you the patch I started thinking along
> similar lines, about completely getting rid of PrefServiceSyncable.
> I'll take a better look to see if that might actually be possible.
>
> Cheers,
> Jói
>
>
> On Thu, Jan 31, 2013 at 3:30 PM,  <mnissler@chromium.org> wrote:
>> Preview and plan for future work looks good, thanks!
>>
>>
>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>> File chrome/browser/prefs/pref_registry_syncable.cc (right):
>>
>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>> chrome/browser/prefs/pref_registry_syncable.cc:190: PrefSyncStatus
>> sync_status) {
>> Some thoughts here: We were thinking of splitting the syncable stuff
>> away from PrefService entirely and drive PrefModelAssociator via pref
>> change notifications like everything else. When we do that, we might
>> consider getting rid of PrefServiceSyncable as well, keep the
>> name-to-syncable-flag-map here in PrefRegistrySyncable and pass the
>> PrefRegistrySyncable reference to PrefModelAssociator at construction
>> time.
>>
>> I haven't thought through all the details, but if this works out, it
>> would further modularize and simplify things.
>>
>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>> File chrome/browser/prefs/pref_registry_syncable.h (right):
>>
>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>> chrome/browser/prefs/pref_registry_syncable.h:24: typedef
>> base::Callback<void(const char* path)> SyncableRegistrationCallback;
>> Callback needs #include or forward-declaration
>>
>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>> chrome/browser/prefs/pref_registry_syncable.h:95:
>> SyncableRegistrationCallback callback_;
>> DISALLOW_COPY_AND_ASSIGN
>>
>> https://codereview.chromium.org/12079097/

[Jói, 2013-02-01 12:57:51]

One more thing to note:  There is extremely limited use of the
PrefServiceSyncable::IsSyncing and Add/RemoveObserver mechanisms; just
a couple of places in ChromeOS and Ash according to my perusal of the
code.  If these could be removed (doesn't seem obvious, but perhaps
with help from owners of that code), it would simplify point (b) down
to exposing a sync::SyncableService via a method such as
Profile::GetPrefSyncableService().

Cheers,
Jói


On Fri, Feb 1, 2013 at 12:54 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> I've taken a better look.  TL:DR; I'm planning to go ahead with the
> current change as planned, but getting rid of PrefServiceSyncable
> later is feasible.  More details below.
>
> Getting rid of PrefServiceSyncable is doable, but orthogonal to the
> current change, and if you/we decide to do it, it will be much easier
> to think about and implement once this first change has been
> committed.  Doing it all at once wouldn't simplify the current change,
> I don't see much double-work if any.
>
> PrefServiceSyncable has three main areas of responsibility remaining
> once my current PrefRegistrySyncable change goes in.  Here's how they
> might be moved to other places:
> a) Tell PrefModelAssociator about registration changes. This could
> move to PrefRegistrySyncable.
> b) Notify observers of syncing changes, and provide access to the
> SyncableService. This could move to PrefModelAssociator, which would
> be exposed as e.g. Profile::GetPrefSyncService. One issue is that the
> PrefModelAssociator interface is wider than what is currently exposed
> via PrefSyncableService.
> c) Know how to construct an incognito PrefService, and provide some
> additional safety around that (see
> PrefServiceSyncable::UpdateCommandLineStore). This could potentially
> move to ProfileImpl.
>
> Given the above, I'm ambivalent about whether getting rid of
> PrefServiceSyncable is worth it.  If it is done, then more thought
> should go into how PrefModelAssociator should be exposed to users, and
> you move some knowledge that is specific to this subtype of a
> PrefService away from a separate class and to ProfileImpl (c).
>
> Cheers,
> Jói
>
>
>
> On Thu, Jan 31, 2013 at 3:37 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Thanks Mattias.  After sending you the patch I started thinking along
>> similar lines, about completely getting rid of PrefServiceSyncable.
>> I'll take a better look to see if that might actually be possible.
>>
>> Cheers,
>> Jói
>>
>>
>> On Thu, Jan 31, 2013 at 3:30 PM,  <mnissler@chromium.org> wrote:
>>> Preview and plan for future work looks good, thanks!
>>>
>>>
>>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>>> File chrome/browser/prefs/pref_registry_syncable.cc (right):
>>>
>>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>>> chrome/browser/prefs/pref_registry_syncable.cc:190: PrefSyncStatus
>>> sync_status) {
>>> Some thoughts here: We were thinking of splitting the syncable stuff
>>> away from PrefService entirely and drive PrefModelAssociator via pref
>>> change notifications like everything else. When we do that, we might
>>> consider getting rid of PrefServiceSyncable as well, keep the
>>> name-to-syncable-flag-map here in PrefRegistrySyncable and pass the
>>> PrefRegistrySyncable reference to PrefModelAssociator at construction
>>> time.
>>>
>>> I haven't thought through all the details, but if this works out, it
>>> would further modularize and simplify things.
>>>
>>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>>> File chrome/browser/prefs/pref_registry_syncable.h (right):
>>>
>>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>>> chrome/browser/prefs/pref_registry_syncable.h:24: typedef
>>> base::Callback<void(const char* path)> SyncableRegistrationCallback;
>>> Callback needs #include or forward-declaration
>>>
>>>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
>>> chrome/browser/prefs/pref_registry_syncable.h:95:
>>> SyncableRegistrationCallback callback_;
>>> DISALLOW_COPY_AND_ASSIGN
>>>
>>> https://codereview.chromium.org/12079097/

[Mattias Nissler (ping if slow), 2013-02-01 13:25:42]

All this sounds good to me!

On Fri, Feb 1, 2013 at 1:57 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> One more thing to note:  There is extremely limited use of the
> PrefServiceSyncable::IsSyncing and Add/RemoveObserver mechanisms; just
> a couple of places in ChromeOS and Ash according to my perusal of the
> code.  If these could be removed (doesn't seem obvious, but perhaps
> with help from owners of that code), it would simplify point (b) down
> to exposing a sync::SyncableService via a method such as
> Profile::GetPrefSyncableService().
>
> Cheers,
> Jói
>
>
> On Fri, Feb 1, 2013 at 12:54 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> > I've taken a better look.  TL:DR; I'm planning to go ahead with the
> > current change as planned, but getting rid of PrefServiceSyncable
> > later is feasible.  More details below.
> >
> > Getting rid of PrefServiceSyncable is doable, but orthogonal to the
> > current change, and if you/we decide to do it, it will be much easier
> > to think about and implement once this first change has been
> > committed.  Doing it all at once wouldn't simplify the current change,
> > I don't see much double-work if any.
>

Yes, I agree. Smaller CLs are better, and cleaning up the syncable stuff
and getting rid of PrefServiceSyncable entirely is definitely an orthogonal
topic to switching all consumers from PrefServiceSyncable to PrefService.


> >
> > PrefServiceSyncable has three main areas of responsibility remaining
> > once my current PrefRegistrySyncable change goes in.  Here's how they
> > might be moved to other places:
> > a) Tell PrefModelAssociator about registration changes. This could
> > move to PrefRegistrySyncable.
>

PrefModelAssociator could maybe just register as a PrefStore::Observer on
UserPrefStore.

 > b) Notify observers of syncing changes, and provide access to the
> > SyncableService. This could move to PrefModelAssociator, which would
> > be exposed as e.g. Profile::GetPrefSyncService. One issue is that the
> > PrefModelAssociator interface is wider than what is currently exposed
> > via PrefSyncableService.
>

Sounds good.


>  > c) Know how to construct an incognito PrefService, and provide some
> > additional safety around that (see
> > PrefServiceSyncable::UpdateCommandLineStore). This could potentially
> > move to ProfileImpl.
>

That sounds good as well.


>  >
> > Given the above, I'm ambivalent about whether getting rid of
> > PrefServiceSyncable is worth it.  If it is done, then more thought
> > should go into how PrefModelAssociator should be exposed to users, and
> > you move some knowledge that is specific to this subtype of a
> > PrefService away from a separate class and to ProfileImpl (c).
>

That's a good point - the sync people always preferred to build the syncing
functionality "right into the feature", so we may just keep
PrefServiceSyncable as an implementation detail.


> >
> > Cheers,
> > Jói
> >
> >
> >
> > On Thu, Jan 31, 2013 at 3:37 PM, Jói Sigurðsson <joi@chromium.org>
> wrote:
> >> Thanks Mattias.  After sending you the patch I started thinking along
> >> similar lines, about completely getting rid of PrefServiceSyncable.
> >> I'll take a better look to see if that might actually be possible.
> >>
> >> Cheers,
> >> Jói
> >>
> >>
> >> On Thu, Jan 31, 2013 at 3:30 PM,  <mnissler@chromium.org> wrote:
> >>> Preview and plan for future work looks good, thanks!
> >>>
> >>>
> >>>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> >>> File chrome/browser/prefs/pref_registry_syncable.cc (right):
> >>>
> >>>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> >>> chrome/browser/prefs/pref_registry_syncable.cc:190: PrefSyncStatus
> >>> sync_status) {
> >>> Some thoughts here: We were thinking of splitting the syncable stuff
> >>> away from PrefService entirely and drive PrefModelAssociator via pref
> >>> change notifications like everything else. When we do that, we might
> >>> consider getting rid of PrefServiceSyncable as well, keep the
> >>> name-to-syncable-flag-map here in PrefRegistrySyncable and pass the
> >>> PrefRegistrySyncable reference to PrefModelAssociator at construction
> >>> time.
> >>>
> >>> I haven't thought through all the details, but if this works out, it
> >>> would further modularize and simplify things.
> >>>
> >>>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> >>> File chrome/browser/prefs/pref_registry_syncable.h (right):
> >>>
> >>>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> >>> chrome/browser/prefs/pref_registry_syncable.h:24: typedef
> >>> base::Callback<void(const char* path)> SyncableRegistrationCallback;
> >>> Callback needs #include or forward-declaration
> >>>
> >>>
>
https://codereview.chromium.org/12079097/diff/2001/chrome/browser/prefs/pref_...
> >>> chrome/browser/prefs/pref_registry_syncable.h:95:
> >>> SyncableRegistrationCallback callback_;
> >>> DISALLOW_COPY_AND_ASSIGN
> >>>
> >>> https://codereview.chromium.org/12079097/
>

[Jói, 2013-02-05 16:50:29]

Hi Mattias,

I think this is ready for review.  It's compiling on all bots, but there is a
sync integration test failing that I need to investigate.

A huge portion of this change is just boilerplate changes for RegisterUserPrefs
changes to take a PrefRegistrySyncable instead of a PrefServiceSyncable, and to
change the parameter name for these from typically 'prefs' to 'registry'.  If
you like, I could run my code review helper tool to generate separate patch
files for the "boilerplate" vs. the "for manual review" parts of this, plus a
list of "interesting" files; let me know if this would be useful.

Cheers,
Jói

[Jói, 2013-02-06 15:00:18]

Hi Mattias,

I'm pretty sure I found the issue with the sync_integration_tests
(tryjobs are pending). It was that I wasn't telling the
PrefModelAssociator about registrations of syncable prefs prior to
PrefServiceSyncable creation.  You'll see a small diff from patch set
3 to patch set 4, isolated to PrefRegistrySyncable and
PrefServiceSyncable, to fix this.

Cheers,
Jói



On Tue, Feb 5, 2013 at 4:50 PM,  <joi@chromium.org> wrote:
> Hi Mattias,
>
> I think this is ready for review.  It's compiling on all bots, but there is
> a
> sync integration test failing that I need to investigate.
>
> A huge portion of this change is just boilerplate changes for
> RegisterUserPrefs
> changes to take a PrefRegistrySyncable instead of a PrefServiceSyncable, and
> to
> change the parameter name for these from typically 'prefs' to 'registry'.
> If
> you like, I could run my code review helper tool to generate separate patch
> files for the "boilerplate" vs. the "for manual review" parts of this, plus
> a
> list of "interesting" files; let me know if this would be useful.
>
> Cheers,
> Jói
>
>
> https://codereview.chromium.org/12079097/

[Mattias Nissler (ping if slow), 2013-02-06 15:02:18]

Sounds good, I'm actually in the process of flushing my review queue for
today, will get to this in a few.

On Wed, Feb 6, 2013 at 3:59 PM, Jói Sigurðsson <joi@chromium.org> wrote:

> Hi Mattias,
>
> I'm pretty sure I found the issue with the sync_integration_tests
> (tryjobs are pending). It was that I wasn't telling the
> PrefModelAssociator about registrations of syncable prefs prior to
> PrefServiceSyncable creation.  You'll see a small diff from patch set
> 3 to patch set 4, isolated to PrefRegistrySyncable and
> PrefServiceSyncable, to fix this.
>
> Cheers,
> Jói
>
>
>
> On Tue, Feb 5, 2013 at 4:50 PM,  <joi@chromium.org> wrote:
> > Hi Mattias,
> >
> > I think this is ready for review.  It's compiling on all bots, but there
> is
> > a
> > sync integration test failing that I need to investigate.
> >
> > A huge portion of this change is just boilerplate changes for
> > RegisterUserPrefs
> > changes to take a PrefRegistrySyncable instead of a PrefServiceSyncable,
> and
> > to
> > change the parameter name for these from typically 'prefs' to 'registry'.
> > If
> > you like, I could run my code review helper tool to generate separate
> patch
> > files for the "boilerplate" vs. the "for manual review" parts of this,
> plus
> > a
> > list of "interesting" files; let me know if this would be useful.
> >
> > Cheers,
> > Jói
> >
> >
> > https://codereview.chromium.org/12079097/
>

[Jói, 2013-02-06 15:08:56]

Thanks Mattias.  There is no big rush, I have other stuff to work on
and it's a big change.  Would love to get it finalized before the
weekend.

Cheers,
Jói


On Wed, Feb 6, 2013 at 3:01 PM, Mattias Nissler <mnissler@chromium.org> wrote:
> Sounds good, I'm actually in the process of flushing my review queue for
> today, will get to this in a few.
>
>
> On Wed, Feb 6, 2013 at 3:59 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>>
>> Hi Mattias,
>>
>> I'm pretty sure I found the issue with the sync_integration_tests
>> (tryjobs are pending). It was that I wasn't telling the
>> PrefModelAssociator about registrations of syncable prefs prior to
>> PrefServiceSyncable creation.  You'll see a small diff from patch set
>> 3 to patch set 4, isolated to PrefRegistrySyncable and
>> PrefServiceSyncable, to fix this.
>>
>> Cheers,
>> Jói
>>
>>
>>
>> On Tue, Feb 5, 2013 at 4:50 PM,  <joi@chromium.org> wrote:
>> > Hi Mattias,
>> >
>> > I think this is ready for review.  It's compiling on all bots, but there
>> > is
>> > a
>> > sync integration test failing that I need to investigate.
>> >
>> > A huge portion of this change is just boilerplate changes for
>> > RegisterUserPrefs
>> > changes to take a PrefRegistrySyncable instead of a PrefServiceSyncable,
>> > and
>> > to
>> > change the parameter name for these from typically 'prefs' to
>> > 'registry'.
>> > If
>> > you like, I could run my code review helper tool to generate separate
>> > patch
>> > files for the "boilerplate" vs. the "for manual review" parts of this,
>> > plus
>> > a
>> > list of "interesting" files; let me know if this would be useful.
>> >
>> > Cheers,
>> > Jói
>> >
>> >
>> > https://codereview.chromium.org/12079097/
>
>

[Mattias Nissler (ping if slow), 2013-02-06 17:53:33]

Made it through - I'm surprised how many people abuse the pref system :)

The only significant remaining concern is our story on who gets access to a
PrefServiceSyncable pointer. I had hoped that we could isolate that type
essentially to ProfileImpl, but it seems like that's not easily possible.

https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
File base/prefs/public/pref_member.h (right):

https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
base/prefs/public/pref_member.h:291: //
PrefMember<base::FilePath>::UpdateValueInternal not being defined when
unrelated changes?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_prompt_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_prompt_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
Should be pref_service_base.h instead? But the plan for that was to go away in
favor of a single PrefService type, correct? In that case, no change needed.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
chrome/browser/chromeos/preferences.cc:103: if
(prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
TODO here?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:140:
PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
can we use a TestingPrefService instance here instead (to avoid the
proliferation of the PrefServiceSyncable type)?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:147:
PrefServiceSyncable* otr_prefs = builder.CreateSyncable(otr_registry);
same here

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:383:
pref_service()->DeprecatedGetPrefRegistry()));
I guess you can just drop this and line 374 and remove the pref_service() getter
and things will still work?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs_unittest.cc:826:
prefs()->pref_service()->DeprecatedGetPrefRegistry());
This is an interesting case. One option could be to switch the test to using
actually-registered prefs. Another option could be to pass in a pre-constructed
PrefRegistrySyncable to ExtensionPrefs::Create.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/menu_manager_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/menu_manager_unittest.cc:54:
prefs_.pref_service()->CommitPendingWrite();
pref_service() should return a PrefService instead of a PrefServiceSyncable
here.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:12: #include
"chrome/browser/prefs/pref_service_syncable.h"
one is enough. or did you mean to include pref_registry_syncable.h?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/test_extension_prefs.cc:97:
scoped_refptr<PrefRegistrySyncable> registry(new PrefRegistrySyncable);
As mentioned elsewhere, it would probably be nice to be able to pass in the
registry to use here.

As a step in the right direction, we could make the registry a member of
TestExtensionPrefs and let the tests access it?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/test_extension_prefs.cc:99:
ExtensionPrefs::RegisterUserPrefs(registry);
For good measure, can we swap lines 98 and 99?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_prefs.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_prefs.h:9: class PrefRegistrySyncable;
required?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
chrome/browser/prefs/incognito_mode_prefs.cc:57: DCHECK(registry);
nit: not needed, we crash below if the pointer is NULL.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_syncable.h:105: std::set<std::string>
syncable_preferences_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:138: if
(pref_sync_associator_.IsPrefRegistered(path)) {
nit: no curlies required

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:24: // PrefServiceSyncable is
primarily an implementation detail of
Can you elaborate on the "primarily"? I thought the idea was to remove the
PrefServiceSyncable type from an code consuming PrefService instances. I see a
couple tests still using PrefServiceSyncable here - are these the only
offenders?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
File chrome/browser/profiles/gaia_info_update_service.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
chrome/browser/profiles/gaia_info_update_service.h:17: class
PrefRegistrySyncable;
needed?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:88:
prefs_(PrefServiceSyncable::IncognitoFromProfile(real_profile)),
Do we really need a PrefServiceSyncable typed pointer? Can we possibly declare
prefs_ to be of type PrefService?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
chrome/browser/profiles/profile.cc:134: return
ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged(); //
revert

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_keyed_base_factory.cc:87:
DeprecatedRegisterUserPrefs(prefs, registry);
Yikes, hacks over hacks here. I'll have to think a bit more about registrations
coming from PKSs.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
chrome/browser/sync/invalidations/invalidator_storage.cc:103: if (registry) {
TODO: Move to registration function.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_components_factory_impl.cc:285: return
PrefServiceSyncable::FromProfile(
non-scoring comment: This seems to be the non-test call site  I found that
actually needs a PrefServiceSyncable instead of a PrefService. The answer for
this one could be to split out the syncing stuff.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:65: bool enable_by_default =
!prefs->HasPrefPath(prefs::kSyncHasSetupCompleted);
TODO: Remove |prefs| parameter.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:223:
PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
Oh, this is the OnIsSyncingChanged stuff. I'll have to look at that separately
to find out why the hell we have that, no need to wait for that.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:268:
prefs::kBrowserActionContainerWidth))
TODO: registrations up front.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:358:
prefs->ClearPref(kTouchScreenFlingAccelerationAdjustment);
non-scoring comment: This should be part of migration.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
chrome/browser/ui/gtk/browser_window_gtk.cc:1493: // is the case in Valgrind
tests).
TODO remove prefs parameter.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:134: // an empty string as the
default.
TODO up-front registrations

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:578:
RegisterPrefsToMigrate(registry);
In this case, we could easily fix the code right away: Call
RegisterPrefsToMigrate from RegisterUserPrefs, remove the Unregister call below,
and stop passing in |registry|. WDYT? I'm also OK with a TODO.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:9: #include
"chrome/browser/prefs/pref_registry_syncable.h"
needed?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
chrome/browser/web_resource/notification_promo.cc:323:
PrefRegistrySyncable::UNSYNCABLE_PREF);
TODO remove prefs parameter, this should be part of the migration code.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
chrome/browser/web_resource/promo_resource_service.cc:78:
prefs->ClearPref(prefs::kNtpPromoResourceCacheUpdate);
TODO: move to migration code.

https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
File chrome/test/base/testing_pref_service.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
chrome/test/base/testing_pref_service.cc:113:
local_state_.DeprecatedGetPrefRegistry()));
local_state_ is a TestingPrefServiceSimple - local_state_.registry() should just
work?

https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h#newc...
net/base/mime_util.h:18: NET_EXPORT bool GetMimeTypeFromExtension(const
base::FilePath::StringType& ext,
unrelated changes?

https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog.h (right):

https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog.h:109:
std::vector<std::vector<base::FilePath::StringType> > extensions;
unrelated changes?

[Jói, 2013-02-07 14:52:32]

Thanks Mattias.  Addressed all review comments, although I did not act on every
one, we can discuss.

Cheers,
Jói

https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
File base/prefs/public/pref_member.h (right):

https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
base/prefs/public/pref_member.h:291: //
PrefMember<base::FilePath>::UpdateValueInternal not being defined when
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> unrelated changes?

These were to fix a compilation snafu after a merge, but were unnecessary it
turns out. Removed from this change.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_prompt_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_prompt_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> Should be pref_service_base.h instead? But the plan for that was to go away in
> favor of a single PrefService type, correct? In that case, no change needed.

That's right, the plan is for PrefServiceBase to go away so I'm preferring to
include the concrete type.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
chrome/browser/chromeos/preferences.cc:103: if
(prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO here?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:140:
PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> can we use a TestingPrefService instance here instead (to avoid the
> proliferation of the PrefServiceSyncable type)?

That's challenging due to the overlay-on-original relationship that the
otr_prefs need to have with the regular_prefs.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:147:
PrefServiceSyncable* otr_prefs = builder.CreateSyncable(otr_registry);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> same here

ditto

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:383:
pref_service()->DeprecatedGetPrefRegistry()));
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> I guess you can just drop this and line 374 and remove the pref_service()
getter
> and things will still work?

I did the same as Joe Thomas contributed in
https://codereview.chromium.org/12211026/diff/1/chrome/browser/custom_handler...
(pending commit), i.e. remove the code and add a CHECK(profile_->GetPrefs()).

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs_unittest.cc:826:
prefs()->pref_service()->DeprecatedGetPrefRegistry());
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> This is an interesting case. One option could be to switch the test to using
> actually-registered prefs. Another option could be to pass in a
pre-constructed
> PrefRegistrySyncable to ExtensionPrefs::Create.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/menu_manager_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/menu_manager_unittest.cc:54:
prefs_.pref_service()->CommitPendingWrite();
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> pref_service() should return a PrefService instead of a PrefServiceSyncable
> here.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:12: #include
"chrome/browser/prefs/pref_service_syncable.h"
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> one is enough. or did you mean to include pref_registry_syncable.h?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
File chrome/browser/extensions/test_extension_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/test_extension_prefs.cc:97:
scoped_refptr<PrefRegistrySyncable> registry(new PrefRegistrySyncable);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> As mentioned elsewhere, it would probably be nice to be able to pass in the
> registry to use here.
> 
> As a step in the right direction, we could make the registry a member of
> TestExtensionPrefs and let the tests access it?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
chrome/browser/extensions/test_extension_prefs.cc:99:
ExtensionPrefs::RegisterUserPrefs(registry);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> For good measure, can we swap lines 98 and 99?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
File chrome/browser/geolocation/geolocation_prefs.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
chrome/browser/geolocation/geolocation_prefs.h:9: class PrefRegistrySyncable;
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> required?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
File chrome/browser/prefs/incognito_mode_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
chrome/browser/prefs/incognito_mode_prefs.cc:57: DCHECK(registry);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> nit: not needed, we crash below if the pointer is NULL.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_registry_syncable.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_registry_syncable.h:105: std::set<std::string>
syncable_preferences_;
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.cc:138: if
(pref_sync_associator_.IsPrefRegistered(path)) {
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> nit: no curlies required

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:24: // PrefServiceSyncable is
primarily an implementation detail of
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> Can you elaborate on the "primarily"? I thought the idea was to remove the
> PrefServiceSyncable type from an code consuming PrefService instances. I see a
> couple tests still using PrefServiceSyncable here - are these the only
> offenders?

Anyone who needs to know about the sync integration for PrefService is likely to
need to know about the PrefServiceSyncable type.  I've updated the comment.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
File chrome/browser/profiles/gaia_info_update_service.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
chrome/browser/profiles/gaia_info_update_service.h:17: class
PrefRegistrySyncable;
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:88:
prefs_(PrefServiceSyncable::IncognitoFromProfile(real_profile)),
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> Do we really need a PrefServiceSyncable typed pointer? Can we possibly declare
> prefs_ to be of type PrefService?

I think we probably could, but we'd be casting it in a few more places and it'd
be less typesafe. Note that profile.h and related headers don't #include the
PrefServiceSyncable header, they just forward-declare the class so that they can
store a pointer to it, so changing the type wouldn't hide it any better than
it's hidden now...

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
chrome/browser/profiles/profile.cc:134: return
ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged(); //
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> revert

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_keyed_base_factory.cc:87:
DeprecatedRegisterUserPrefs(prefs, registry);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> Yikes, hacks over hacks here. I'll have to think a bit more about
registrations
> coming from PKSs.

Yeah, when I ran into this I thought "oops" :)  But better to clean such things
up in separate smaller changes I think.   

I figured that for now, we'd be better off with a separate DeprecatedXyz method
that gets the PrefService parameter, rather than passing PrefService in the main
method, so that people don't start accidentally introducing more dependencies.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
File chrome/browser/sync/invalidations/invalidator_storage.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
chrome/browser/sync/invalidations/invalidator_storage.cc:103: if (registry) {
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO: Move to registration function.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_components_factory_impl.cc:285: return
PrefServiceSyncable::FromProfile(
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> non-scoring comment: This seems to be the non-test call site  I found that
> actually needs a PrefServiceSyncable instead of a PrefService. The answer for
> this one could be to split out the syncing stuff.

OK.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
File chrome/browser/sync/sync_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
chrome/browser/sync/sync_prefs.cc:65: bool enable_by_default =
!prefs->HasPrefPath(prefs::kSyncHasSetupCompleted);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO: Remove |prefs| parameter.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:223:
PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> Oh, this is the OnIsSyncingChanged stuff. I'll have to look at that separately
> to find out why the hell we have that, no need to wait for that.

OK.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:268:
prefs::kBrowserActionContainerWidth))
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO: registrations up front.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:358:
prefs->ClearPref(kTouchScreenFlingAccelerationAdjustment);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> non-scoring comment: This should be part of migration.

Agreed, I think many of the "special" cases we're seeing in RegisterPrefs and
RegisterUserPrefs can be moved to separate MigratePrefs and MigrateUserPrefs
that get called post PrefService construction.  I wonder if there should be more
specific support for migration than the "Register then Clear" or "Register then
Clear then Unregister" combos I'm seeing in various places.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
chrome/browser/ui/gtk/browser_window_gtk.cc:1493: // is the case in Valgrind
tests).
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO remove prefs parameter.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:134: // an empty string as the
default.
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO up-front registrations

This is called from RegisterUserPrefs so I think it is already up-front.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:578:
RegisterPrefsToMigrate(registry);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> In this case, we could easily fix the code right away: Call
> RegisterPrefsToMigrate from RegisterUserPrefs, remove the Unregister call
below,
> and stop passing in |registry|. WDYT? I'm also OK with a TODO.

Won't Migrate(User)Prefs functions generally need both the PrefService and the
PrefRegistry?

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:9: #include
"chrome/browser/prefs/pref_registry_syncable.h"
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> needed?

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
File chrome/browser/web_resource/notification_promo.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
chrome/browser/web_resource/notification_promo.cc:323:
PrefRegistrySyncable::UNSYNCABLE_PREF);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO remove prefs parameter, this should be part of the migration code.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
File chrome/browser/web_resource/promo_resource_service.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
chrome/browser/web_resource/promo_resource_service.cc:78:
prefs->ClearPref(prefs::kNtpPromoResourceCacheUpdate);
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> TODO: move to migration code.

Done.

https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
File chrome/test/base/testing_pref_service.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
chrome/test/base/testing_pref_service.cc:113:
local_state_.DeprecatedGetPrefRegistry()));
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> local_state_ is a TestingPrefServiceSimple - local_state_.registry() should
just
> work?

Done.

https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h
File net/base/mime_util.h (right):

https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h#newc...
net/base/mime_util.h:18: NET_EXPORT bool GetMimeTypeFromExtension(const
base::FilePath::StringType& ext,
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> unrelated changes?

Correct, as in pref_member.h. Removed.

https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
File ui/shell_dialogs/select_file_dialog.h (right):

https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
ui/shell_dialogs/select_file_dialog.h:109:
std::vector<std::vector<base::FilePath::StringType> > extensions;
On 2013/02/06 17:53:33, Mattias Nissler wrote:
> unrelated changes?

Yes, as in pref_member.h. Removed.

[Jói, 2013-02-08 10:51:15]

Gentle ping - all trybots are currently looking green so it'd be great
to land this soon.

Cheers,
Jói


On Thu, Feb 7, 2013 at 2:52 PM,  <joi@chromium.org> wrote:
> Thanks Mattias.  Addressed all review comments, although I did not act on
> every
> one, we can discuss.
>
> Cheers,
> Jói
>
>
>
>
>
https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
> File base/prefs/public/pref_member.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
> base/prefs/public/pref_member.h:291: //
> PrefMember<base::FilePath>::UpdateValueInternal not being defined when
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> unrelated changes?
>
>
> These were to fix a compilation snafu after a merge, but were
> unnecessary it turns out. Removed from this change.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
> File chrome/browser/bookmarks/bookmark_prompt_prefs.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
> chrome/browser/bookmarks/bookmark_prompt_prefs.cc:8: #include
> "chrome/browser/prefs/pref_service.h"
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> Should be pref_service_base.h instead? But the plan for that was to go
>
> away in
>>
>> favor of a single PrefService type, correct? In that case, no change
>
> needed.
>
> That's right, the plan is for PrefServiceBase to go away so I'm
> preferring to include the concrete type.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/preferences.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
> chrome/browser/chromeos/preferences.cc:103: if
> (prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO here?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
> File
> chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
>
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:140:
> PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> can we use a TestingPrefService instance here instead (to avoid the
>> proliferation of the PrefServiceSyncable type)?
>
>
> That's challenging due to the overlay-on-original relationship that the
> otr_prefs need to have with the regular_prefs.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
>
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:147:
> PrefServiceSyncable* otr_prefs = builder.CreateSyncable(otr_registry);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> same here
>
>
> ditto
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
> File
> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:383:
> pref_service()->DeprecatedGetPrefRegistry()));
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> I guess you can just drop this and line 374 and remove the
>
> pref_service() getter
>>
>> and things will still work?
>
>
> I did the same as Joe Thomas contributed in
>
https://codereview.chromium.org/12211026/diff/1/chrome/browser/custom_handler...
> (pending commit), i.e. remove the code and add a
> CHECK(profile_->GetPrefs()).
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> File chrome/browser/extensions/extension_prefs_unittest.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> chrome/browser/extensions/extension_prefs_unittest.cc:826:
> prefs()->pref_service()->DeprecatedGetPrefRegistry());
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> This is an interesting case. One option could be to switch the test to
>
> using
>>
>> actually-registered prefs. Another option could be to pass in a
>
> pre-constructed
>>
>> PrefRegistrySyncable to ExtensionPrefs::Create.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> File chrome/browser/extensions/menu_manager_unittest.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> chrome/browser/extensions/menu_manager_unittest.cc:54:
> prefs_.pref_service()->CommitPendingWrite();
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> pref_service() should return a PrefService instead of a
>
> PrefServiceSyncable
>>
>> here.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:12:
> #include "chrome/browser/prefs/pref_service_syncable.h"
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> one is enough. or did you mean to include pref_registry_syncable.h?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> File chrome/browser/extensions/test_extension_prefs.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> chrome/browser/extensions/test_extension_prefs.cc:97:
> scoped_refptr<PrefRegistrySyncable> registry(new PrefRegistrySyncable);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> As mentioned elsewhere, it would probably be nice to be able to pass
>
> in the
>>
>> registry to use here.
>
>
>> As a step in the right direction, we could make the registry a member
>
> of
>>
>> TestExtensionPrefs and let the tests access it?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> chrome/browser/extensions/test_extension_prefs.cc:99:
> ExtensionPrefs::RegisterUserPrefs(registry);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> For good measure, can we swap lines 98 and 99?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
> File chrome/browser/geolocation/geolocation_prefs.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
> chrome/browser/geolocation/geolocation_prefs.h:9: class
> PrefRegistrySyncable;
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> required?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
> File chrome/browser/prefs/incognito_mode_prefs.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
> chrome/browser/prefs/incognito_mode_prefs.cc:57: DCHECK(registry);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> nit: not needed, we crash below if the pointer is NULL.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_registry_syncable.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_registry_syncable.h:105: std::set<std::string>
> syncable_preferences_;
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> DISALLOW_COPY_AND_ASSIGN?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_syncable.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_syncable.cc:138: if
> (pref_sync_associator_.IsPrefRegistered(path)) {
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> nit: no curlies required
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_syncable.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_syncable.h:24: // PrefServiceSyncable
> is primarily an implementation detail of
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> Can you elaborate on the "primarily"? I thought the idea was to remove
>
> the
>>
>> PrefServiceSyncable type from an code consuming PrefService instances.
>
> I see a
>>
>> couple tests still using PrefServiceSyncable here - are these the only
>> offenders?
>
>
> Anyone who needs to know about the sync integration for PrefService is
> likely to need to know about the PrefServiceSyncable type.  I've updated
> the comment.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
> File chrome/browser/profiles/gaia_info_update_service.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
> chrome/browser/profiles/gaia_info_update_service.h:17: class
> PrefRegistrySyncable;
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> needed?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
> File chrome/browser/profiles/off_the_record_profile_impl.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
> chrome/browser/profiles/off_the_record_profile_impl.cc:88:
> prefs_(PrefServiceSyncable::IncognitoFromProfile(real_profile)),
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> Do we really need a PrefServiceSyncable typed pointer? Can we possibly
>
> declare
>>
>> prefs_ to be of type PrefService?
>
>
> I think we probably could, but we'd be casting it in a few more places
> and it'd be less typesafe. Note that profile.h and related headers don't
> #include the PrefServiceSyncable header, they just forward-declare the
> class so that they can store a pointer to it, so changing the type
> wouldn't hide it any better than it's hidden now...
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> chrome/browser/profiles/profile.cc:134: return
> ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged(); //
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> revert
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_keyed_base_factory.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_keyed_base_factory.cc:87:
> DeprecatedRegisterUserPrefs(prefs, registry);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> Yikes, hacks over hacks here. I'll have to think a bit more about
>
> registrations
>>
>> coming from PKSs.
>
>
> Yeah, when I ran into this I thought "oops" :)  But better to clean such
> things up in separate smaller changes I think.
>
> I figured that for now, we'd be better off with a separate DeprecatedXyz
> method that gets the PrefService parameter, rather than passing
> PrefService in the main method, so that people don't start accidentally
> introducing more dependencies.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
> File chrome/browser/sync/invalidations/invalidator_storage.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
> chrome/browser/sync/invalidations/invalidator_storage.cc:103: if
> (registry) {
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO: Move to registration function.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
> File chrome/browser/sync/profile_sync_components_factory_impl.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
> chrome/browser/sync/profile_sync_components_factory_impl.cc:285: return
> PrefServiceSyncable::FromProfile(
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> non-scoring comment: This seems to be the non-test call site  I found
>
> that
>>
>> actually needs a PrefServiceSyncable instead of a PrefService. The
>
> answer for
>>
>> this one could be to split out the syncing stuff.
>
>
> OK.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
> File chrome/browser/sync/sync_prefs.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
> chrome/browser/sync/sync_prefs.cc:65: bool enable_by_default =
> !prefs->HasPrefPath(prefs::kSyncHasSetupCompleted);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO: Remove |prefs| parameter.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
> File
> chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
> chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:223:
> PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> Oh, this is the OnIsSyncingChanged stuff. I'll have to look at that
>
> separately
>>
>> to find out why the hell we have that, no need to wait for that.
>
>
> OK.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
> File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
> chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:268:
> prefs::kBrowserActionContainerWidth))
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO: registrations up front.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
> File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
> chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:358:
> prefs->ClearPref(kTouchScreenFlingAccelerationAdjustment);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> non-scoring comment: This should be part of migration.
>
>
> Agreed, I think many of the "special" cases we're seeing in
> RegisterPrefs and RegisterUserPrefs can be moved to separate
> MigratePrefs and MigrateUserPrefs that get called post PrefService
> construction.  I wonder if there should be more specific support for
> migration than the "Register then Clear" or "Register then Clear then
> Unregister" combos I'm seeing in various places.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
> File chrome/browser/ui/gtk/browser_window_gtk.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
> chrome/browser/ui/gtk/browser_window_gtk.cc:1493: // is the case in
> Valgrind tests).
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO remove prefs parameter.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:134: // an empty string as
> the default.
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO up-front registrations
>
>
> This is called from RegisterUserPrefs so I think it is already up-front.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:578:
> RegisterPrefsToMigrate(registry);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> In this case, we could easily fix the code right away: Call
>> RegisterPrefsToMigrate from RegisterUserPrefs, remove the Unregister
>
> call below,
>>
>> and stop passing in |registry|. WDYT? I'm also OK with a TODO.
>
>
> Won't Migrate(User)Prefs functions generally need both the PrefService
> and the PrefRegistry?
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
> File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
> chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:9: #include
> "chrome/browser/prefs/pref_registry_syncable.h"
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> needed?
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> File chrome/browser/web_resource/notification_promo.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> chrome/browser/web_resource/notification_promo.cc:323:
> PrefRegistrySyncable::UNSYNCABLE_PREF);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO remove prefs parameter, this should be part of the migration
>
> code.
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> File chrome/browser/web_resource/promo_resource_service.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> chrome/browser/web_resource/promo_resource_service.cc:78:
> prefs->ClearPref(prefs::kNtpPromoResourceCacheUpdate);
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> TODO: move to migration code.
>
>
> Done.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
> File chrome/test/base/testing_pref_service.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
> chrome/test/base/testing_pref_service.cc:113:
> local_state_.DeprecatedGetPrefRegistry()));
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> local_state_ is a TestingPrefServiceSimple - local_state_.registry()
>
> should just
>>
>> work?
>
>
> Done.
>
>
> https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h
> File net/base/mime_util.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h#newc...
> net/base/mime_util.h:18: NET_EXPORT bool GetMimeTypeFromExtension(const
> base::FilePath::StringType& ext,
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> unrelated changes?
>
>
> Correct, as in pref_member.h. Removed.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
> File ui/shell_dialogs/select_file_dialog.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
> ui/shell_dialogs/select_file_dialog.h:109:
> std::vector<std::vector<base::FilePath::StringType> > extensions;
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>>
>> unrelated changes?
>
>
> Yes, as in pref_member.h. Removed.
>
> https://codereview.chromium.org/12079097/

[Mattias Nissler (ping if slow), 2013-02-08 11:05:09]

in the process of flushing my review queue...

On Fri, Feb 8, 2013 at 11:50 AM, Jói Sigurðsson <joi@chromium.org> wrote:

> Gentle ping - all trybots are currently looking green so it'd be great
> to land this soon.
>
> Cheers,
> Jói
>
>
> On Thu, Feb 7, 2013 at 2:52 PM,  <joi@chromium.org> wrote:
> > Thanks Mattias.  Addressed all review comments, although I did not act on
> > every
> > one, we can discuss.
> >
> > Cheers,
> > Jói
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
> > File base/prefs/public/pref_member.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/base/prefs/public/pref_me...
> > base/prefs/public/pref_member.h:291: //
> > PrefMember<base::FilePath>::UpdateValueInternal not being defined when
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> unrelated changes?
> >
> >
> > These were to fix a compilation snafu after a merge, but were
> > unnecessary it turns out. Removed from this change.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
> > File chrome/browser/bookmarks/bookmark_prompt_prefs.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/bookmarks/...
> > chrome/browser/bookmarks/bookmark_prompt_prefs.cc:8: #include
> > "chrome/browser/prefs/pref_service.h"
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> Should be pref_service_base.h instead? But the plan for that was to go
> >
> > away in
> >>
> >> favor of a single PrefService type, correct? In that case, no change
> >
> > needed.
> >
> > That's right, the plan is for PrefServiceBase to go away so I'm
> > preferring to include the concrete type.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
> > File chrome/browser/chromeos/preferences.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/chromeos/p...
> > chrome/browser/chromeos/preferences.cc:103: if
> > (prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO here?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
> > File
> >
> chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
> >
>
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:140:
> > PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> can we use a TestingPrefService instance here instead (to avoid the
> >> proliferation of the PrefServiceSyncable type)?
> >
> >
> > That's challenging due to the overlay-on-original relationship that the
> > otr_prefs need to have with the regular_prefs.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
> >
>
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:147:
> > PrefServiceSyncable* otr_prefs = builder.CreateSyncable(otr_registry);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> same here
> >
> >
> > ditto
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
> > File
> > chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
> > chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:383:
> > pref_service()->DeprecatedGetPrefRegistry()));
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> I guess you can just drop this and line 374 and remove the
> >
> > pref_service() getter
> >>
> >> and things will still work?
> >
> >
> > I did the same as Joe Thomas contributed in
> >
>
https://codereview.chromium.org/12211026/diff/1/chrome/browser/custom_handler...
> > (pending commit), i.e. remove the code and add a
> > CHECK(profile_->GetPrefs()).
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > File chrome/browser/extensions/extension_prefs_unittest.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > chrome/browser/extensions/extension_prefs_unittest.cc:826:
> > prefs()->pref_service()->DeprecatedGetPrefRegistry());
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> This is an interesting case. One option could be to switch the test to
> >
> > using
> >>
> >> actually-registered prefs. Another option could be to pass in a
> >
> > pre-constructed
> >>
> >> PrefRegistrySyncable to ExtensionPrefs::Create.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > File chrome/browser/extensions/menu_manager_unittest.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > chrome/browser/extensions/menu_manager_unittest.cc:54:
> > prefs_.pref_service()->CommitPendingWrite();
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> pref_service() should return a PrefService instead of a
> >
> > PrefServiceSyncable
> >>
> >> here.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:12:
> > #include "chrome/browser/prefs/pref_service_syncable.h"
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> one is enough. or did you mean to include pref_registry_syncable.h?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > File chrome/browser/extensions/test_extension_prefs.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > chrome/browser/extensions/test_extension_prefs.cc:97:
> > scoped_refptr<PrefRegistrySyncable> registry(new PrefRegistrySyncable);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> As mentioned elsewhere, it would probably be nice to be able to pass
> >
> > in the
> >>
> >> registry to use here.
> >
> >
> >> As a step in the right direction, we could make the registry a member
> >
> > of
> >>
> >> TestExtensionPrefs and let the tests access it?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/extensions...
> > chrome/browser/extensions/test_extension_prefs.cc:99:
> > ExtensionPrefs::RegisterUserPrefs(registry);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> For good measure, can we swap lines 98 and 99?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
> > File chrome/browser/geolocation/geolocation_prefs.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/geolocatio...
> > chrome/browser/geolocation/geolocation_prefs.h:9: class
> > PrefRegistrySyncable;
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> required?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
> > File chrome/browser/prefs/incognito_mode_prefs.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/inco...
> > chrome/browser/prefs/incognito_mode_prefs.cc:57: DCHECK(registry);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> nit: not needed, we crash below if the pointer is NULL.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_registry_syncable.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_registry_syncable.h:105: std::set<std::string>
> > syncable_preferences_;
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> DISALLOW_COPY_AND_ASSIGN?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_service_syncable.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_service_syncable.cc:138: if
> > (pref_sync_associator_.IsPrefRegistered(path)) {
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> nit: no curlies required
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> > File chrome/browser/prefs/pref_service_syncable.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> > chrome/browser/prefs/pref_service_syncable.h:24: // PrefServiceSyncable
> > is primarily an implementation detail of
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> Can you elaborate on the "primarily"? I thought the idea was to remove
> >
> > the
> >>
> >> PrefServiceSyncable type from an code consuming PrefService instances.
> >
> > I see a
> >>
> >> couple tests still using PrefServiceSyncable here - are these the only
> >> offenders?
> >
> >
> > Anyone who needs to know about the sync integration for PrefService is
> > likely to need to know about the PrefServiceSyncable type.  I've updated
> > the comment.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
> > File chrome/browser/profiles/gaia_info_update_service.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/g...
> > chrome/browser/profiles/gaia_info_update_service.h:17: class
> > PrefRegistrySyncable;
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> needed?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
> > File chrome/browser/profiles/off_the_record_profile_impl.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
> > chrome/browser/profiles/off_the_record_profile_impl.cc:88:
> > prefs_(PrefServiceSyncable::IncognitoFromProfile(real_profile)),
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> Do we really need a PrefServiceSyncable typed pointer? Can we possibly
> >
> > declare
> >>
> >> prefs_ to be of type PrefService?
> >
> >
> > I think we probably could, but we'd be casting it in a few more places
> > and it'd be less typesafe. Note that profile.h and related headers don't
> > #include the PrefServiceSyncable header, they just forward-declare the
> > class so that they can store a pointer to it, so changing the type
> > wouldn't hide it any better than it's hidden now...
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile.cc:134: return
> > ProfileSyncService::IsSyncEnabled() && !prefs.IsManaged(); //
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> revert
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> > File chrome/browser/profiles/profile_keyed_base_factory.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> > chrome/browser/profiles/profile_keyed_base_factory.cc:87:
> > DeprecatedRegisterUserPrefs(prefs, registry);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> Yikes, hacks over hacks here. I'll have to think a bit more about
> >
> > registrations
> >>
> >> coming from PKSs.
> >
> >
> > Yeah, when I ran into this I thought "oops" :)  But better to clean such
> > things up in separate smaller changes I think.
> >
> > I figured that for now, we'd be better off with a separate DeprecatedXyz
> > method that gets the PrefService parameter, rather than passing
> > PrefService in the main method, so that people don't start accidentally
> > introducing more dependencies.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
> > File chrome/browser/sync/invalidations/invalidator_storage.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/inval...
> > chrome/browser/sync/invalidations/invalidator_storage.cc:103: if
> > (registry) {
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO: Move to registration function.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
> > File chrome/browser/sync/profile_sync_components_factory_impl.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/profi...
> > chrome/browser/sync/profile_sync_components_factory_impl.cc:285: return
> > PrefServiceSyncable::FromProfile(
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> non-scoring comment: This seems to be the non-test call site  I found
> >
> > that
> >>
> >> actually needs a PrefServiceSyncable instead of a PrefService. The
> >
> > answer for
> >>
> >> this one could be to split out the syncing stuff.
> >
> >
> > OK.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
> > File chrome/browser/sync/sync_prefs.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/sync/sync_...
> > chrome/browser/sync/sync_prefs.cc:65: bool enable_by_default =
> > !prefs->HasPrefPath(prefs::kSyncHasSetupCompleted);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO: Remove |prefs| parameter.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
> > File
> > chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
> > chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:223:
> > PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> Oh, this is the OnIsSyncingChanged stuff. I'll have to look at that
> >
> > separately
> >>
> >> to find out why the hell we have that, no need to wait for that.
> >
> >
> > OK.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
> > File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm
> > (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/cocoa/e...
> > chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:268:
> > prefs::kBrowserActionContainerWidth))
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO: registrations up front.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
> > File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
> > chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:358:
> > prefs->ClearPref(kTouchScreenFlingAccelerationAdjustment);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> non-scoring comment: This should be part of migration.
> >
> >
> > Agreed, I think many of the "special" cases we're seeing in
> > RegisterPrefs and RegisterUserPrefs can be moved to separate
> > MigratePrefs and MigrateUserPrefs that get called post PrefService
> > construction.  I wonder if there should be more specific support for
> > migration than the "Register then Clear" or "Register then Clear then
> > Unregister" combos I'm seeing in various places.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
> > File chrome/browser/ui/gtk/browser_window_gtk.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gtk/bro...
> > chrome/browser/ui/gtk/browser_window_gtk.cc:1493: // is the case in
> > Valgrind tests).
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO remove prefs parameter.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> > File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> > chrome/browser/ui/prefs/prefs_tab_helper.cc:134: // an empty string as
> > the default.
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO up-front registrations
> >
> >
> > This is called from RegisterUserPrefs so I think it is already up-front.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> > chrome/browser/ui/prefs/prefs_tab_helper.cc:578:
> > RegisterPrefsToMigrate(registry);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> In this case, we could easily fix the code right away: Call
> >> RegisterPrefsToMigrate from RegisterUserPrefs, remove the Unregister
> >
> > call below,
> >>
> >> and stop passing in |registry|. WDYT? I'm also OK with a TODO.
> >
> >
> > Won't Migrate(User)Prefs functions generally need both the PrefService
> > and the PrefRegistry?
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
> > File chrome/browser/ui/sync/one_click_signin_helper_unittest.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/sync/on...
> > chrome/browser/ui/sync/one_click_signin_helper_unittest.cc:9: #include
> > "chrome/browser/prefs/pref_registry_syncable.h"
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> needed?
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> > File chrome/browser/web_resource/notification_promo.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> > chrome/browser/web_resource/notification_promo.cc:323:
> > PrefRegistrySyncable::UNSYNCABLE_PREF);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO remove prefs parameter, this should be part of the migration
> >
> > code.
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> > File chrome/browser/web_resource/promo_resource_service.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/web_resour...
> > chrome/browser/web_resource/promo_resource_service.cc:78:
> > prefs->ClearPref(prefs::kNtpPromoResourceCacheUpdate);
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> TODO: move to migration code.
> >
> >
> > Done.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
> > File chrome/test/base/testing_pref_service.cc (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/chrome/test/base/testing_...
> > chrome/test/base/testing_pref_service.cc:113:
> > local_state_.DeprecatedGetPrefRegistry()));
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> local_state_ is a TestingPrefServiceSimple - local_state_.registry()
> >
> > should just
> >>
> >> work?
> >
> >
> > Done.
> >
> >
> > https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h
> > File net/base/mime_util.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/net/base/mime_util.h#newc...
> > net/base/mime_util.h:18: NET_EXPORT bool GetMimeTypeFromExtension(const
> > base::FilePath::StringType& ext,
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> unrelated changes?
> >
> >
> > Correct, as in pref_member.h. Removed.
> >
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
> > File ui/shell_dialogs/select_file_dialog.h (right):
> >
> >
>
https://codereview.chromium.org/12079097/diff/29005/ui/shell_dialogs/select_f...
> > ui/shell_dialogs/select_file_dialog.h:109:
> > std::vector<std::vector<base::FilePath::StringType> > extensions;
> > On 2013/02/06 17:53:33, Mattias Nissler wrote:
> >>
> >> unrelated changes?
> >
> >
> > Yes, as in pref_member.h. Removed.
> >
> > https://codereview.chromium.org/12079097/
>

[Mattias Nissler (ping if slow), 2013-02-08 11:26:56]

LGTM - no further changes needed, but I put a few musings on the points we
discussed.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:140:
PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > can we use a TestingPrefService instance here instead (to avoid the
> > proliferation of the PrefServiceSyncable type)?
> 
> That's challenging due to the overlay-on-original relationship that the
> otr_prefs need to have with the regular_prefs.

It seems in this test they actually don't (which I believe is fine for the
test), that's why I'm asking.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:383:
pref_service()->DeprecatedGetPrefRegistry()));
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > I guess you can just drop this and line 374 and remove the pref_service()
> getter
> > and things will still work?
> 
> I did the same as Joe Thomas contributed in
>
https://codereview.chromium.org/12211026/diff/1/chrome/browser/custom_handler...
> (pending commit), i.e. remove the code and add a CHECK(profile_->GetPrefs()).

Good solution, thanks!

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
File chrome/browser/prefs/pref_service_syncable.h (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
chrome/browser/prefs/pref_service_syncable.h:24: // PrefServiceSyncable is
primarily an implementation detail of
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > Can you elaborate on the "primarily"? I thought the idea was to remove the
> > PrefServiceSyncable type from an code consuming PrefService instances. I see
a
> > couple tests still using PrefServiceSyncable here - are these the only
> > offenders?
> 
> Anyone who needs to know about the sync integration for PrefService is likely
to
> need to know about the PrefServiceSyncable type.  I've updated the comment.

Thanks!

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_impl.cc:88:
prefs_(PrefServiceSyncable::IncognitoFromProfile(real_profile)),
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > Do we really need a PrefServiceSyncable typed pointer? Can we possibly
declare
> > prefs_ to be of type PrefService?
> 
> I think we probably could, but we'd be casting it in a few more places and
it'd
> be less typesafe. Note that profile.h and related headers don't #include the
> PrefServiceSyncable header, they just forward-declare the class so that they
can
> store a pointer to it, so changing the type wouldn't hide it any better than
> it's hidden now...

OK for now.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_keyed_base_factory.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
chrome/browser/profiles/profile_keyed_base_factory.cc:87:
DeprecatedRegisterUserPrefs(prefs, registry);
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > Yikes, hacks over hacks here. I'll have to think a bit more about
> registrations
> > coming from PKSs.
> 
> Yeah, when I ran into this I thought "oops" :)  But better to clean such
things
> up in separate smaller changes I think.   
> 
> I figured that for now, we'd be better off with a separate DeprecatedXyz
method
> that gets the PrefService parameter, rather than passing PrefService in the
main
> method, so that people don't start accidentally introducing more dependencies.

So, I wonder whether we'd be better off if we just had all the PKSF that are
compiled in register their prefs when a profile initializes. Admittedly this
will register prefs that are not used (because the corresponding PKS is not
instantiated), but that's a price I'd be willing to pay.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:358:
prefs->ClearPref(kTouchScreenFlingAccelerationAdjustment);
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > non-scoring comment: This should be part of migration.
> 
> Agreed, I think many of the "special" cases we're seeing in RegisterPrefs and
> RegisterUserPrefs can be moved to separate MigratePrefs and MigrateUserPrefs
> that get called post PrefService construction.  I wonder if there should be
more
> specific support for migration than the "Register then Clear" or "Register
then
> Clear then Unregister" combos I'm seeing in various places.

+1 to adding that migration code path.

Regarding the registration/unregistration, I wouldn't worry too much: What's the
harm in keeping the registrations for deprecated prefs in the regular
RegisterPrefs() code paths? Once the migration code gets removed, we can remove
the registrations as well.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:134: // an empty string as the
default.
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > TODO up-front registrations
> 
> This is called from RegisterUserPrefs so I think it is already up-front.

True, my bad.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
chrome/browser/ui/prefs/prefs_tab_helper.cc:578:
RegisterPrefsToMigrate(registry);
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > In this case, we could easily fix the code right away: Call
> > RegisterPrefsToMigrate from RegisterUserPrefs, remove the Unregister call
> below,
> > and stop passing in |registry|. WDYT? I'm also OK with a TODO.
> 
> Won't Migrate(User)Prefs functions generally need both the PrefService and the
> PrefRegistry?

Unless we follow through with the up-front-registrations policy and do
registrations even for deprecated prefs up front. I don't see much harm in that.

[Jói, 2013-02-08 11:30:08]

Thanks Mattias.  I'll try to get this big change checked in, and then
will think about follow-ups based on your comments below.  I think
you're right that up-front registration of migration prefs is probably
fine, although we might want to keep it somewhat separate so that it's
easier to identify which prefs can be removed.

Cheers,
Jói


On Fri, Feb 8, 2013 at 11:26 AM,  <mnissler@chromium.org> wrote:
> LGTM - no further changes needed, but I put a few musings on the points we
> discussed.
>
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
> File
> chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/content_se...
>
chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:140:
> PrefServiceSyncable* regular_prefs = builder.CreateSyncable(registry);
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > can we use a TestingPrefService instance here instead (to avoid the
>> > proliferation of the PrefServiceSyncable type)?
>
>
>> That's challenging due to the overlay-on-original relationship that
>
> the
>>
>> otr_prefs need to have with the regular_prefs.
>
>
> It seems in this test they actually don't (which I believe is fine for
> the test), that's why I'm asking.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
> File
> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
> (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/custom_han...
> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:383:
> pref_service()->DeprecatedGetPrefRegistry()));
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > I guess you can just drop this and line 374 and remove the
>
> pref_service()
>>
>> getter
>> > and things will still work?
>
>
>> I did the same as Joe Thomas contributed in
>
>
>
https://codereview.chromium.org/12211026/diff/1/chrome/browser/custom_handler...
>>
>> (pending commit), i.e. remove the code and add a
>
> CHECK(profile_->GetPrefs()).
>
> Good solution, thanks!
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> File chrome/browser/prefs/pref_service_syncable.h (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/prefs/pref...
> chrome/browser/prefs/pref_service_syncable.h:24: // PrefServiceSyncable
> is primarily an implementation detail of
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > Can you elaborate on the "primarily"? I thought the idea was to
>
> remove the
>>
>> > PrefServiceSyncable type from an code consuming PrefService
>
> instances. I see a
>>
>> > couple tests still using PrefServiceSyncable here - are these the
>
> only
>>
>> > offenders?
>
>
>> Anyone who needs to know about the sync integration for PrefService is
>
> likely to
>>
>> need to know about the PrefServiceSyncable type.  I've updated the
>
> comment.
>
> Thanks!
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
> File chrome/browser/profiles/off_the_record_profile_impl.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/o...
> chrome/browser/profiles/off_the_record_profile_impl.cc:88:
> prefs_(PrefServiceSyncable::IncognitoFromProfile(real_profile)),
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > Do we really need a PrefServiceSyncable typed pointer? Can we
>
> possibly declare
>>
>> > prefs_ to be of type PrefService?
>
>
>> I think we probably could, but we'd be casting it in a few more places
>
> and it'd
>>
>> be less typesafe. Note that profile.h and related headers don't
>
> #include the
>>
>> PrefServiceSyncable header, they just forward-declare the class so
>
> that they can
>>
>> store a pointer to it, so changing the type wouldn't hide it any
>
> better than
>>
>> it's hidden now...
>
>
> OK for now.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> File chrome/browser/profiles/profile_keyed_base_factory.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/profiles/p...
> chrome/browser/profiles/profile_keyed_base_factory.cc:87:
> DeprecatedRegisterUserPrefs(prefs, registry);
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > Yikes, hacks over hacks here. I'll have to think a bit more about
>> registrations
>> > coming from PKSs.
>
>
>> Yeah, when I ran into this I thought "oops" :)  But better to clean
>
> such things
>>
>> up in separate smaller changes I think.
>
>
>> I figured that for now, we'd be better off with a separate
>
> DeprecatedXyz method
>>
>> that gets the PrefService parameter, rather than passing PrefService
>
> in the main
>>
>> method, so that people don't start accidentally introducing more
>
> dependencies.
>
> So, I wonder whether we'd be better off if we just had all the PKSF that
> are compiled in register their prefs when a profile initializes.
> Admittedly this will register prefs that are not used (because the
> corresponding PKS is not instantiated), but that's a price I'd be
> willing to pay.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
> File chrome/browser/ui/gesture_prefs_observer_factory_aura.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/gesture...
> chrome/browser/ui/gesture_prefs_observer_factory_aura.cc:358:
> prefs->ClearPref(kTouchScreenFlingAccelerationAdjustment);
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > non-scoring comment: This should be part of migration.
>
>
>> Agreed, I think many of the "special" cases we're seeing in
>
> RegisterPrefs and
>>
>> RegisterUserPrefs can be moved to separate MigratePrefs and
>
> MigrateUserPrefs
>>
>> that get called post PrefService construction.  I wonder if there
>
> should be more
>>
>> specific support for migration than the "Register then Clear" or
>
> "Register then
>>
>> Clear then Unregister" combos I'm seeing in various places.
>
>
> +1 to adding that migration code path.
>
> Regarding the registration/unregistration, I wouldn't worry too much:
> What's the harm in keeping the registrations for deprecated prefs in the
> regular RegisterPrefs() code paths? Once the migration code gets
> removed, we can remove the registrations as well.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:134: // an empty string as
> the default.
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > TODO up-front registrations
>
>
>> This is called from RegisterUserPrefs so I think it is already
>
> up-front.
>
> True, my bad.
>
>
>
https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/prefs/p...
> chrome/browser/ui/prefs/prefs_tab_helper.cc:578:
> RegisterPrefsToMigrate(registry);
> On 2013/02/07 14:52:32, Jói wrote:
>>
>> On 2013/02/06 17:53:33, Mattias Nissler wrote:
>> > In this case, we could easily fix the code right away: Call
>> > RegisterPrefsToMigrate from RegisterUserPrefs, remove the Unregister
>
> call
>>
>> below,
>> > and stop passing in |registry|. WDYT? I'm also OK with a TODO.
>
>
>> Won't Migrate(User)Prefs functions generally need both the PrefService
>
> and the
>>
>> PrefRegistry?
>
>
> Unless we follow through with the up-front-registrations policy and do
> registrations even for deprecated prefs up front. I don't see much harm
> in that.
>
> https://codereview.chromium.org/12079097/

[Jói, 2013-02-08 12:54:14]

TBR=ben@chromium.org for usage updates across the codebase. Mattias already
reviewed the whole change, as the primary bits are Prefs changes.

Cheers,
Jói

[commit-bot: I haz the power, 2013-02-08 14:40:40]

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

[Mattias Nissler (ping if slow), 2013-02-08 14:49:18]

Another followup - FYI.

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12079097/diff/29005/chrome/browser/ui/ash/lau...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:223:
PrefServiceSyncable::FromProfile(profile_)->RemoveObserver(this);
On 2013/02/07 14:52:32, Jói wrote:
> On 2013/02/06 17:53:33, Mattias Nissler wrote:
> > Oh, this is the OnIsSyncingChanged stuff. I'll have to look at that
separately
> > to find out why the hell we have that, no need to wait for that.
> 
> OK.

FWIW, I've looked into this some more. I've found that the IsSyncing is used for
the following: Waiting for the sync value of a pref to arrive and then not
applying it blindly but making the decision for what pref value to use actually
based on additional factors. This pattern is present in both
MaybePropagatePrefToLocal in chrome_launcher_controller_per_{browser,app}.cc and
chromeos::Preferences::ForceNaturalScrollDefault() (which seem to be the only
consumers of this).

I think we could cover this need nicely way by allowing custom pref merge logic
for synced-in data. Now that we have PrefRegistrySyncable, that should be
relatively simple by adding something like this:

typedef base::Callback<scoped_ptr<Value>(const Value* local_value, const Value*
sync_value)) MergeCallback;
void PrefRegistrySyncable::RegisterMergeCallback(const char* pref_name, const
MergeCallback& callback);

This would also allow us to get rid of the duplicated prefs used by
ChromeLauncherController.

[commit-bot: I haz the power, 2013-02-08 15:28:29]

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

[commit-bot: I haz the power, 2013-02-08 15:29:58]

Failed to apply patch for chrome/browser/chromeos/preferences.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/preferences.cc
  Hunk #4 FAILED at 300.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/preferences.cc.rej

Patch:       chrome/browser/chromeos/preferences.cc
Index: chrome/browser/chromeos/preferences.cc
diff --git a/chrome/browser/chromeos/preferences.cc
b/chrome/browser/chromeos/preferences.cc
index
3d4e98b4dc725acd2807556a6074f52da9ac6de3..4c796d739ad4d9382c1dd53c67645838a2abf223
100644
--- a/chrome/browser/chromeos/preferences.cc
+++ b/chrome/browser/chromeos/preferences.cc
@@ -27,7 +27,8 @@
 #include "chrome/browser/chromeos/system/power_manager_settings.h"
 #include "chrome/browser/chromeos/system/statistics_provider.h"
 #include "chrome/browser/download/download_util.h"
-#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/prefs/pref_registry_syncable.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
@@ -57,7 +58,9 @@ Preferences::~Preferences() {
 }
 
 // static
-void Preferences::RegisterUserPrefs(PrefServiceSyncable* prefs) {
+void Preferences::RegisterUserPrefs(PrefService* prefs,
+                                    PrefRegistrySyncable* registry) {
+  // TODO(joi): Get rid of need for PrefService parameter.
   std::string hardware_keyboard_id;
   // TODO(yusukes): Remove the runtime hack.
   if (base::chromeos::IsRunningOnChromeOS()) {
@@ -71,223 +74,224 @@ void Preferences::RegisterUserPrefs(PrefServiceSyncable*
prefs) {
     hardware_keyboard_id = "xkb:us::eng";  // only for testing.
   }
 
-  prefs->RegisterBooleanPref(prefs::kTapToClickEnabled,
-                             true,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kTapDraggingEnabled,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerClick,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerSwipe,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(
+  registry->RegisterBooleanPref(prefs::kTapToClickEnabled,
+                                true,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kTapDraggingEnabled,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerClick,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerSwipe,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(
       prefs::kNaturalScroll,
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kNaturalScrollDefault),
-      PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kPrimaryMouseButtonRight,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kLabsMediaplayerEnabled,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kLabsAdvancedFilesystemEnabled,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
+      PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kPrimaryMouseButtonRight,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kLabsMediaplayerEnabled,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kLabsAdvancedFilesystemEnabled,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
   // Check if the accessibility prefs are already registered, which can happen
   // in WizardController::RegisterPrefs. We still want to try to register
   // the prefs here in case of Chrome/Linux with ChromeOS=1.
   if (prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
-    prefs->RegisterBooleanPref(prefs::kSpokenFeedbackEnabled,
-                               false,
-                               PrefServiceSyncable::UNSYNCABLE_PREF);
+    registry->RegisterBooleanPref(prefs::kSpokenFeedbackEnabled,
+                                  false,
+                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
   }
   if (prefs->FindPreference(prefs::kHighContrastEnabled) == NULL) {
-    prefs->RegisterBooleanPref(prefs::kHighContrastEnabled,
-                               false,
-                               PrefServiceSyncable::UNSYNCABLE_PREF);
+    registry->RegisterBooleanPref(prefs::kHighContrastEnabled,
+                                  false,
+                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
   }
   if (prefs->FindPreference(prefs::kVirtualKeyboardEnabled) == NULL) {
-    prefs->RegisterBooleanPref(prefs::kVirtualKeyboardEnabled,
-                               false,
-                               PrefServiceSyncable::UNSYNCABLE_PREF);
-  }
-  prefs->RegisterBooleanPref(prefs::kScreenMagnifierEnabled,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kScreenMagnifierType,
-                             ash::kDefaultMagnifierType,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterDoublePref(prefs::kScreenMagnifierScale,
-                            std::numeric_limits<double>::min(),
-                            PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kShouldAlwaysShowAccessibilityMenu,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kMouseSensitivity,
-                             3,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kTouchpadSensitivity,
-                             3,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kUse24HourClock,
-                             base::GetHourClockType() == base::k24HourClock,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kDisableDrive,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kDisableDriveOverCellular,
-                             true,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kDisableDriveHostedFiles,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
+    registry->RegisterBooleanPref(prefs::kVirtualKeyboardEnabled,
+                                  false,
+                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
+  }
+  registry->RegisterBooleanPref(prefs::kScreenMagnifierEnabled,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterIntegerPref(prefs::kScreenMagnifierType,
+                                ash::kDefaultMagnifierType,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterDoublePref(prefs::kScreenMagnifierScale,
+                               std::numeric_limits<double>::min(),
+                               PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kShouldAlwaysShowAccessibilityMenu,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterIntegerPref(prefs::kMouseSensitivity,
+                                3,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterIntegerPref(prefs::kTouchpadSensitivity,
+                                3,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kUse24HourClock,
+                                base::GetHourClockType() == base::k24HourClock,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kDisableDrive,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kDisableDriveOverCellular,
+                                true,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kDisableDriveHostedFiles,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
   // We don't sync prefs::kLanguageCurrentInputMethod and PreviousInputMethod
   // because they're just used to track the logout state of the device.
-  prefs->RegisterStringPref(prefs::kLanguageCurrentInputMethod,
+  registry->RegisterStringPref(prefs::kLanguageCurrentInputMethod,
+                               "",
+                               PrefRegistrySyncable:…
(message too large)

[commit-bot: I haz the power, 2013-02-08 18:08:05]

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

[commit-bot: I haz the power, 2013-02-08 18:09:47]

Failed to apply patch for chrome/browser/chromeos/preferences.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/preferences.cc
  Hunk #4 FAILED at 300.
  1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/preferences.cc.rej

Patch:       chrome/browser/chromeos/preferences.cc
Index: chrome/browser/chromeos/preferences.cc
diff --git a/chrome/browser/chromeos/preferences.cc
b/chrome/browser/chromeos/preferences.cc
index
444364ca6d57e64bdb6423bcc4e084e3768205a0..e042599069ff4d0b12db67ad1d631772352939b5
100644
--- a/chrome/browser/chromeos/preferences.cc
+++ b/chrome/browser/chromeos/preferences.cc
@@ -27,7 +27,8 @@
 #include "chrome/browser/chromeos/system/power_manager_settings.h"
 #include "chrome/browser/chromeos/system/statistics_provider.h"
 #include "chrome/browser/download/download_util.h"
-#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/prefs/pref_registry_syncable.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
@@ -57,7 +58,9 @@ Preferences::~Preferences() {
 }
 
 // static
-void Preferences::RegisterUserPrefs(PrefServiceSyncable* prefs) {
+void Preferences::RegisterUserPrefs(PrefService* prefs,
+                                    PrefRegistrySyncable* registry) {
+  // TODO(joi): Get rid of need for PrefService parameter.
   std::string hardware_keyboard_id;
   // TODO(yusukes): Remove the runtime hack.
   if (base::chromeos::IsRunningOnChromeOS()) {
@@ -71,223 +74,224 @@ void Preferences::RegisterUserPrefs(PrefServiceSyncable*
prefs) {
     hardware_keyboard_id = "xkb:us::eng";  // only for testing.
   }
 
-  prefs->RegisterBooleanPref(prefs::kTapToClickEnabled,
-                             true,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kTapDraggingEnabled,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerClick,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerSwipe,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(
+  registry->RegisterBooleanPref(prefs::kTapToClickEnabled,
+                                true,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kTapDraggingEnabled,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerClick,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerSwipe,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(
       prefs::kNaturalScroll,
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kNaturalScrollDefault),
-      PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kPrimaryMouseButtonRight,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kLabsMediaplayerEnabled,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kLabsAdvancedFilesystemEnabled,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
+      PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kPrimaryMouseButtonRight,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kLabsMediaplayerEnabled,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kLabsAdvancedFilesystemEnabled,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
   // Check if the accessibility prefs are already registered, which can happen
   // in WizardController::RegisterPrefs. We still want to try to register
   // the prefs here in case of Chrome/Linux with ChromeOS=1.
   if (prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
-    prefs->RegisterBooleanPref(prefs::kSpokenFeedbackEnabled,
-                               false,
-                               PrefServiceSyncable::UNSYNCABLE_PREF);
+    registry->RegisterBooleanPref(prefs::kSpokenFeedbackEnabled,
+                                  false,
+                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
   }
   if (prefs->FindPreference(prefs::kHighContrastEnabled) == NULL) {
-    prefs->RegisterBooleanPref(prefs::kHighContrastEnabled,
-                               false,
-                               PrefServiceSyncable::UNSYNCABLE_PREF);
+    registry->RegisterBooleanPref(prefs::kHighContrastEnabled,
+                                  false,
+                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
   }
   if (prefs->FindPreference(prefs::kVirtualKeyboardEnabled) == NULL) {
-    prefs->RegisterBooleanPref(prefs::kVirtualKeyboardEnabled,
-                               false,
-                               PrefServiceSyncable::UNSYNCABLE_PREF);
-  }
-  prefs->RegisterBooleanPref(prefs::kScreenMagnifierEnabled,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kScreenMagnifierType,
-                             ash::kDefaultMagnifierType,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterDoublePref(prefs::kScreenMagnifierScale,
-                            std::numeric_limits<double>::min(),
-                            PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kShouldAlwaysShowAccessibilityMenu,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kMouseSensitivity,
-                             3,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterIntegerPref(prefs::kTouchpadSensitivity,
-                             3,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kUse24HourClock,
-                             base::GetHourClockType() == base::k24HourClock,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kDisableDrive,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kDisableDriveOverCellular,
-                             true,
-                             PrefServiceSyncable::SYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kDisableDriveHostedFiles,
-                             false,
-                             PrefServiceSyncable::SYNCABLE_PREF);
+    registry->RegisterBooleanPref(prefs::kVirtualKeyboardEnabled,
+                                  false,
+                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
+  }
+  registry->RegisterBooleanPref(prefs::kScreenMagnifierEnabled,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterIntegerPref(prefs::kScreenMagnifierType,
+                                ash::kDefaultMagnifierType,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterDoublePref(prefs::kScreenMagnifierScale,
+                               std::numeric_limits<double>::min(),
+                               PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kShouldAlwaysShowAccessibilityMenu,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterIntegerPref(prefs::kMouseSensitivity,
+                                3,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterIntegerPref(prefs::kTouchpadSensitivity,
+                                3,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kUse24HourClock,
+                                base::GetHourClockType() == base::k24HourClock,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kDisableDrive,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kDisableDriveOverCellular,
+                                true,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kDisableDriveHostedFiles,
+                                false,
+                                PrefRegistrySyncable::SYNCABLE_PREF);
   // We don't sync prefs::kLanguageCurrentInputMethod and PreviousInputMethod
   // because they're just used to track the logout state of the device.
-  prefs->RegisterStringPref(prefs::kLanguageCurrentInputMethod,
+  registry->RegisterStringPref(prefs::kLanguageCurrentInputMethod,
+                               "",
+                               PrefRegistrySyncable:…
(message too large)

[Jói, 2013-02-08 18:18:42]

Wow, that was fun - the change that I had to sync to got reverted
_one_ revision after the change I merged to :D  Let's try this again
shall we? :)

On Fri, Feb 8, 2013 at 6:09 PM,  <commit-bot@chromium.org> wrote:
> Failed to apply patch for chrome/browser/chromeos/preferences.cc:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file chrome/browser/chromeos/preferences.cc
>   Hunk #4 FAILED at 300.
>   1 out of 4 hunks FAILED -- saving rejects to file
> chrome/browser/chromeos/preferences.cc.rej
>
> Patch:       chrome/browser/chromeos/preferences.cc
> Index: chrome/browser/chromeos/preferences.cc
> diff --git a/chrome/browser/chromeos/preferences.cc
> b/chrome/browser/chromeos/preferences.cc
> index
>
444364ca6d57e64bdb6423bcc4e084e3768205a0..e042599069ff4d0b12db67ad1d631772352939b5
>
> 100644
> --- a/chrome/browser/chromeos/preferences.cc
> +++ b/chrome/browser/chromeos/preferences.cc
> @@ -27,7 +27,8 @@
>  #include "chrome/browser/chromeos/system/power_manager_settings.h"
>  #include "chrome/browser/chromeos/system/statistics_provider.h"
>  #include "chrome/browser/download/download_util.h"
> -#include "chrome/browser/prefs/pref_service.h"
> +#include "chrome/browser/prefs/pref_registry_syncable.h"
> +#include "chrome/browser/prefs/pref_service_syncable.h"
>  #include "chrome/browser/prefs/scoped_user_pref_update.h"
>  #include "chrome/common/chrome_notification_types.h"
>  #include "chrome/common/chrome_switches.h"
> @@ -57,7 +58,9 @@ Preferences::~Preferences() {
>  }
>
>  // static
> -void Preferences::RegisterUserPrefs(PrefServiceSyncable* prefs) {
> +void Preferences::RegisterUserPrefs(PrefService* prefs,
> +                                    PrefRegistrySyncable* registry) {
> +  // TODO(joi): Get rid of need for PrefService parameter.
>    std::string hardware_keyboard_id;
>    // TODO(yusukes): Remove the runtime hack.
>    if (base::chromeos::IsRunningOnChromeOS()) {
> @@ -71,223 +74,224 @@ void
> Preferences::RegisterUserPrefs(PrefServiceSyncable*
> prefs) {
>      hardware_keyboard_id = "xkb:us::eng";  // only for testing.
>    }
>
> -  prefs->RegisterBooleanPref(prefs::kTapToClickEnabled,
> -                             true,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kTapDraggingEnabled,
> -                             false,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerClick,
> -                             false,
> -                             PrefServiceSyncable::UNSYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerSwipe,
> -                             false,
> -                             PrefServiceSyncable::UNSYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(
> +  registry->RegisterBooleanPref(prefs::kTapToClickEnabled,
> +                                true,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kTapDraggingEnabled,
> +                                false,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerClick,
> +                                false,
> +                                PrefRegistrySyncable::UNSYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kEnableTouchpadThreeFingerSwipe,
> +                                false,
> +                                PrefRegistrySyncable::UNSYNCABLE_PREF);
> +  registry->RegisterBooleanPref(
>        prefs::kNaturalScroll,
>        CommandLine::ForCurrentProcess()->HasSwitch(
>            switches::kNaturalScrollDefault),
> -      PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kPrimaryMouseButtonRight,
> -                             false,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kLabsMediaplayerEnabled,
> -                             false,
> -                             PrefServiceSyncable::UNSYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kLabsAdvancedFilesystemEnabled,
> -                             false,
> -                             PrefServiceSyncable::UNSYNCABLE_PREF);
> +      PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kPrimaryMouseButtonRight,
> +                                false,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kLabsMediaplayerEnabled,
> +                                false,
> +                                PrefRegistrySyncable::UNSYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kLabsAdvancedFilesystemEnabled,
> +                                false,
> +                                PrefRegistrySyncable::UNSYNCABLE_PREF);
>    // Check if the accessibility prefs are already registered, which can
> happen
>    // in WizardController::RegisterPrefs. We still want to try to register
>    // the prefs here in case of Chrome/Linux with ChromeOS=1.
>    if (prefs->FindPreference(prefs::kSpokenFeedbackEnabled) == NULL) {
> -    prefs->RegisterBooleanPref(prefs::kSpokenFeedbackEnabled,
> -                               false,
> -                               PrefServiceSyncable::UNSYNCABLE_PREF);
> +    registry->RegisterBooleanPref(prefs::kSpokenFeedbackEnabled,
> +                                  false,
> +                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
>    }
>    if (prefs->FindPreference(prefs::kHighContrastEnabled) == NULL) {
> -    prefs->RegisterBooleanPref(prefs::kHighContrastEnabled,
> -                               false,
> -                               PrefServiceSyncable::UNSYNCABLE_PREF);
> +    registry->RegisterBooleanPref(prefs::kHighContrastEnabled,
> +                                  false,
> +                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
>    }
>    if (prefs->FindPreference(prefs::kVirtualKeyboardEnabled) == NULL) {
> -    prefs->RegisterBooleanPref(prefs::kVirtualKeyboardEnabled,
> -                               false,
> -                               PrefServiceSyncable::UNSYNCABLE_PREF);
> -  }
> -  prefs->RegisterBooleanPref(prefs::kScreenMagnifierEnabled,
> -                             false,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterIntegerPref(prefs::kScreenMagnifierType,
> -                             ash::kDefaultMagnifierType,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterDoublePref(prefs::kScreenMagnifierScale,
> -                            std::numeric_limits<double>::min(),
> -                            PrefServiceSyncable::UNSYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kShouldAlwaysShowAccessibilityMenu,
> -                             false,
> -                             PrefServiceSyncable::UNSYNCABLE_PREF);
> -  prefs->RegisterIntegerPref(prefs::kMouseSensitivity,
> -                             3,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterIntegerPref(prefs::kTouchpadSensitivity,
> -                             3,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kUse24HourClock,
> -                             base::GetHourClockType() ==
> base::k24HourClock,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kDisableDrive,
> -                             false,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kDisableDriveOverCellular,
> -                             true,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> -  prefs->RegisterBooleanPref(prefs::kDisableDriveHostedFiles,
> -                             false,
> -                             PrefServiceSyncable::SYNCABLE_PREF);
> +    registry->RegisterBooleanPref(prefs::kVirtualKeyboardEnabled,
> +                                  false,
> +                                  PrefRegistrySyncable::UNSYNCABLE_PREF);
> +  }
> +  registry->RegisterBooleanPref(prefs::kScreenMagnifierEnabled,
> +                                false,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterIntegerPref(prefs::kScreenMagnifierType,
> +                                ash::kDefaultMagnifierType,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterDoublePref(prefs::kScreenMagnifierScale,
> +                               std::numeric_limits<double>::min(),
> +                               PrefRegistrySyncable::UNSYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kShouldAlwaysShowAccessibilityMenu,
> +                                false,
> +                                PrefRegistrySyncable::UNSYNCABLE_PREF);
> +  registry->RegisterIntegerPref(prefs::kMouseSensitivity,
> +                                3,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterIntegerPref(prefs::kTouchpadSensitivity,
> +                                3,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kUse24HourClock,
> +                                base::GetHourClockType() ==
> base::k24HourClock,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kDisableDrive,
> +                                false,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kDisableDriveOverCellular,
> +                                true,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
> +  registry->RegisterBooleanPref(prefs::kDisableDriveHostedFiles,
> +                                false,
> +                                PrefRegistrySyncable::SYNCABLE_PREF);
>    // We don't sync prefs::kLanguageCurrentInputMethod and
> PreviousInputMethod
>    // because they're just used to track the logout state of the device.
> -  prefs->RegisterStringPref(prefs::kLanguageCurrentInputMethod,
> +  registry->RegisterStringPref(prefs::kLanguageCurrentInputMethod,
> +                               "",
> +                               PrefRegistrySyncable:…
> (message too large)
>
> https://chromiumcodereview.appspot.com/12079097/

[commit-bot: I haz the power, 2013-02-08 18:39:08]

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

[commit-bot: I haz the power, 2013-02-08 20:43:02]

Failed to apply patch for chrome/browser/sync/sync_prefs.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/sync/sync_prefs.cc
  Hunk #4 FAILED at 66.
  Hunk #5 succeeded at 116 (offset 4 lines).
  Hunk #6 succeeded at 396 (offset 4 lines).
  Hunk #7 succeeded at 404 (offset 4 lines).
  1 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/sync/sync_prefs.cc.rej

Patch:       chrome/browser/sync/sync_prefs.cc
Index: chrome/browser/sync/sync_prefs.cc
diff --git a/chrome/browser/sync/sync_prefs.cc
b/chrome/browser/sync/sync_prefs.cc
index
9aa75fc97a6fd87817252892a4c08280b5596d7e..a9d7dde2e220697077598842b59b307154b83caa
100644
--- a/chrome/browser/sync/sync_prefs.cc
+++ b/chrome/browser/sync/sync_prefs.cc
@@ -9,6 +9,7 @@
 #include "base/string_number_conversions.h"
 #include "base/values.h"
 #include "build/build_config.h"
+#include "chrome/browser/prefs/pref_registry_syncable.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/sync/profile_sync_service.h"
@@ -22,7 +23,7 @@ namespace browser_sync {
 
 SyncPrefObserver::~SyncPrefObserver() {}
 
-SyncPrefs::SyncPrefs(PrefServiceSyncable* pref_service)
+SyncPrefs::SyncPrefs(PrefService* pref_service)
     : pref_service_(pref_service) {
   RegisterPrefGroups();
   // TODO(tim): Create a Mock instead of maintaining the if(!pref_service_)
case
@@ -42,16 +43,18 @@ SyncPrefs::~SyncPrefs() {
 }
 
 // static
-void SyncPrefs::RegisterUserPrefs(PrefServiceSyncable* prefs) {
-  prefs->RegisterBooleanPref(prefs::kSyncHasSetupCompleted,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterBooleanPref(prefs::kSyncSuppressStart,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterInt64Pref(prefs::kSyncLastSyncedTime,
-                           0,
-                           PrefServiceSyncable::UNSYNCABLE_PREF);
+void SyncPrefs::RegisterUserPrefs(PrefService* prefs,
+                                  PrefRegistrySyncable* registry) {
+  // TODO(joi): Remove |prefs| parameter.
+  registry->RegisterBooleanPref(prefs::kSyncHasSetupCompleted,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kSyncSuppressStart,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterInt64Pref(prefs::kSyncLastSyncedTime,
+                              0,
+                              PrefRegistrySyncable::UNSYNCABLE_PREF);
 
   // If you've never synced before, or if you're using Chrome OS or Android,
   // all datatypes are on by default.
@@ -63,34 +66,34 @@ void SyncPrefs::RegisterUserPrefs(PrefServiceSyncable*
prefs) {
   bool enable_by_default = !prefs->HasPrefPath(prefs::kSyncHasSetupCompleted);
 #endif
 
-  prefs->RegisterBooleanPref(prefs::kSyncKeepEverythingSynced,
-                             enable_by_default,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kSyncKeepEverythingSynced,
+                                enable_by_default,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
 
   syncer::ModelTypeSet user_types = syncer::UserTypes();
 
   // Treat bookmarks specially.
-  RegisterDataTypePreferredPref(prefs, syncer::BOOKMARKS, true);
+  RegisterDataTypePreferredPref(registry, syncer::BOOKMARKS, true);
   user_types.Remove(syncer::BOOKMARKS);
 
   for (syncer::ModelTypeSet::Iterator it = user_types.First();
        it.Good(); it.Inc()) {
-    RegisterDataTypePreferredPref(prefs, it.Get(), enable_by_default);
+    RegisterDataTypePreferredPref(registry, it.Get(), enable_by_default);
   }
 
-  prefs->RegisterBooleanPref(prefs::kSyncManaged,
-                             false,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterStringPref(prefs::kSyncEncryptionBootstrapToken,
-                            "",
-                            PrefServiceSyncable::UNSYNCABLE_PREF);
-  prefs->RegisterStringPref(prefs::kSyncKeystoreEncryptionBootstrapToken,
-                            "",
-                            PrefServiceSyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(prefs::kSyncManaged,
+                                false,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterStringPref(prefs::kSyncEncryptionBootstrapToken,
+                               "",
+                               PrefRegistrySyncable::UNSYNCABLE_PREF);
+  registry->RegisterStringPref(prefs::kSyncKeystoreEncryptionBootstrapToken,
+                               "",
+                               PrefRegistrySyncable::UNSYNCABLE_PREF);
 #if defined(OS_CHROMEOS)
-  prefs->RegisterStringPref(prefs::kSyncSpareBootstrapToken,
-                            "",
-                            PrefServiceSyncable::UNSYNCABLE_PREF);
+  registry->RegisterStringPref(prefs::kSyncSpareBootstrapToken,
+                               "",
+                               PrefRegistrySyncable::UNSYNCABLE_PREF);
 #endif
 
   // We will start prompting people about new data types after the launch of
@@ -109,9 +112,9 @@ void SyncPrefs::RegisterUserPrefs(PrefServiceSyncable*
prefs) {
   model_set.Put(syncer::APPS);
   model_set.Put(syncer::TYPED_URLS);
   model_set.Put(syncer::SESSIONS);
-  prefs->RegisterListPref(prefs::kSyncAcknowledgedSyncTypes,
-                          syncer::ModelTypeSetToValue(model_set),
-                          PrefServiceSyncable::UNSYNCABLE_PREF);
+  registry->RegisterListPref(prefs::kSyncAcknowledgedSyncTypes,
+                             syncer::ModelTypeSetToValue(model_set),
+                             PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 // static
@@ -389,7 +392,7 @@ void SyncPrefs::RegisterPrefGroups() {
 }
 
 // static
-void SyncPrefs::RegisterDataTypePreferredPref(PrefServiceSyncable* prefs,
+void SyncPrefs::RegisterDataTypePreferredPref(PrefRegistrySyncable* registry,
                                               syncer::ModelType type,
                                               bool is_preferred) {
   const char* pref_name = GetPrefNameForDataType(type);
@@ -397,8 +400,8 @@ void
SyncPrefs::RegisterDataTypePreferredPref(PrefServiceSyncable* prefs,
     NOTREACHED();
     return;
   }
-  prefs->RegisterBooleanPref(pref_name, is_preferred,
-                             PrefServiceSyncable::UNSYNCABLE_PREF);
+  registry->RegisterBooleanPref(pref_name, is_preferred,
+                                PrefRegistrySyncable::UNSYNCABLE_PREF);
 }
 
 bool SyncPrefs::GetDataTypePreferred(syncer::ModelType type) const {

[commit-bot: I haz the power, 2013-02-09 14:53:28]

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

[tfarina, 2013-02-09 17:06:24]

https://codereview.chromium.org/12079097/diff/23509/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_model_factory.h (right):

https://codereview.chromium.org/12079097/diff/23509/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_model_factory.h:14: class
PrefRegistrySyncable;
you should not need this forward declaration since it's part of the public API
of ProfileKeyedServiceFactory

https://codereview.chromium.org/12079097/diff/23509/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/bookmark_prompt_prefs.cc (right):

https://codereview.chromium.org/12079097/diff/23509/chrome/browser/bookmarks/...
chrome/browser/bookmarks/bookmark_prompt_prefs.cc:8: #include
"chrome/browser/prefs/pref_service.h"
why you need this?

[tfarina, 2013-02-09 17:08:29]

https://codereview.chromium.org/12079097/diff/23509/chrome/browser/bookmarks/...
File chrome/browser/bookmarks/DEPS (right):

https://codereview.chromium.org/12079097/diff/23509/chrome/browser/bookmarks/...
chrome/browser/bookmarks/DEPS:8: "+chrome/browser/common",
c/b/common contains only cancelable_request.h, which is deprecated. Do we expect
to add more classes to this directory? If not, can I remove this entry from this
DEPS file?

[commit-bot: I haz the power, 2013-02-09 17:43:06]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-02-10 10:57:48]

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

[commit-bot: I haz the power, 2013-02-10 10:59:32]

Failed to apply patch for
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
  Hunk #2 FAILED at 330.
  Hunk #3 FAILED at 371.
  2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc.rej

Patch:      
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
Index: chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
diff --git
a/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
b/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
index
72a39f3424864429d0d4c2418603dd5ae931130a..294887a308a3926d35178bc28cd068adfead82e6
100644
--- a/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
+++ b/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
@@ -10,6 +10,8 @@
 #include "base/message_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/prefs/pref_registry_syncable.h"
+#include "chrome/browser/prefs/pref_service_syncable.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/custom_handlers/protocol_handler.h"
 #include "chrome/test/base/testing_browser_process.h"
@@ -328,9 +330,6 @@ class ProtocolHandlerRegistryTest : public testing::Test {
   FakeDelegate* delegate() const { return delegate_; }
   ProtocolHandlerRegistry* registry() { return registry_.get(); }
   TestingProfile* profile() const { return profile_.get(); }
-  // TODO(joi): Check if this can be removed, as well as the call to
-  // SetPrefService in SetUp.
-  PrefServiceSyncable* pref_service() const { return profile_->GetPrefs(); }
   const ProtocolHandler& test_protocol_handler() const {
     return test_protocol_handler_;
   }
@@ -369,14 +368,10 @@ class ProtocolHandlerRegistryTest : public testing::Test {
 
   virtual void SetUp() {
     profile_.reset(new TestingProfile());
-    profile_->SetPrefService(new TestingPrefServiceSyncable());
+    CHECK(profile_->GetPrefs());
     SetUpRegistry(true);
     test_protocol_handler_ =
         CreateProtocolHandler("test", GURL("http://test.com/%s"), "Test");
-
-    // TODO(joi): If pref_service() and the SetPrefService above go,
-    // then this could go.
-    ProtocolHandlerRegistry::RegisterUserPrefs(pref_service());
   }
 
   virtual void TearDown() {

[commit-bot: I haz the power, 2013-02-10 11:51:29]

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

[Jói, 2013-02-10 13:48:50]

Committed manually as r181648, the one test that was failing
linux_chromeos has been failing consistently for most changes on the
trybot.

On Sun, Feb 10, 2013 at 11:51 AM,  <commit-bot@chromium.org> wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/joi@chromium.org/12079097/36034
>
>
> https://chromiumcodereview.appspot.com/12079097/