Convert PrefSyncStatus into PrefRegistrationFlags

Convert PrefSyncStatus into PrefRegistrationFlags

This converts the PrefSyncStatus enum which is passed at registration time of
a syncable pref into a more general "registration flags" mechansim for prefs.
PrefRegistrationFlags is an enum used in a bitmask to indicate different
options which can be passed when registering a pref.

Subclasses of PrefRegistry can use the first 8 bits of PrefRegistrationFlags
to specify their own flags. These are used by PrefRegistrySyncable to
specify whether or not the pref is syncable.

In a subsequent CL we plan to add a flag to indicate whether the pref is
"lossy" (see the bug linked).

BUG=476800
Committed: https://crrev.com/51b41a6f25281c3d7378eb21a69e4fcacc8d13f2
Cr-Commit-Position: refs/heads/master@{#326727}

[raymes, 2015-04-20 06:47:57]

raymes@chromium.org changed reviewers:
+ mnissler@chromium.org

[Mattias Nissler (ping if slow), 2015-04-20 09:33:35]

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_registry.h
File base/prefs/pref_registry.h (right):

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_registr...
base/prefs/pref_registry.h:48: const PrefRegistrationFlagsMap&
registration_flags() const {
I think I put a comment regarding the idea to avoid storing flags if they're all
zero. Thoughts on that?

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_service.cc
File base/prefs/pref_service.cc (right):

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_service...
base/prefs/pref_service.cc:322: if (it ==
pref_registry_->registration_flags().end()) {
I'd prefer having this logic in PrefRegistry, and the latter exposing a
GetRegistrationFlags(const std::string& path) function which you can call here.
Doing so would keep flag handling/defaulting in one place.

https://codereview.chromium.org/1096833003/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/1096833003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.cc:168: NOTREACHED() << "invalid sync
flag";
This is no longer appropriate after your code change in lines 58-59, as this may
now get invoked for prefs that are not marked as syncable.

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.cc (right):

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
components/pref_registry/pref_registry_syncable.cc:36: 
nit: remove extra blank line

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.h (right):

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
components/pref_registry/pref_registry_syncable.h:44: UNSYNCABLE_PREF = 1 << 1,
Do we really need a separate flag value for this? I would have expected this to
map to NO_REGISTRATION_FLAG.

[raymes, 2015-04-21 07:58:54]

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_registry.h
File base/prefs/pref_registry.h (right):

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_registr...
base/prefs/pref_registry.h:48: const PrefRegistrationFlagsMap&
registration_flags() const {
I didn't see a comment, but that sounds fine to me :)

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_service.cc
File base/prefs/pref_service.cc (right):

https://codereview.chromium.org/1096833003/diff/20001/base/prefs/pref_service...
base/prefs/pref_service.cc:322: if (it ==
pref_registry_->registration_flags().end()) {
On 2015/04/20 09:33:34, Mattias Nissler wrote:
> I'd prefer having this logic in PrefRegistry, and the latter exposing a
> GetRegistrationFlags(const std::string& path) function which you can call
here.
> Doing so would keep flag handling/defaulting in one place.

Sounds good but the function isn't used at all for now so I'll just remove it.

https://codereview.chromium.org/1096833003/diff/20001/chrome/browser/prefs/pr...
File chrome/browser/prefs/pref_service_syncable.cc (right):

https://codereview.chromium.org/1096833003/diff/20001/chrome/browser/prefs/pr...
chrome/browser/prefs/pref_service_syncable.cc:168: NOTREACHED() << "invalid sync
flag";
Ahh good catch, done.

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.cc (right):

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
components/pref_registry/pref_registry_syncable.cc:36: 
On 2015/04/20 09:33:34, Mattias Nissler wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.h (right):

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
components/pref_registry/pref_registry_syncable.h:44: UNSYNCABLE_PREF = 1 << 1,
On 2015/04/20 09:33:34, Mattias Nissler wrote:
> Do we really need a separate flag value for this? I would have expected this
to
> map to NO_REGISTRATION_FLAG.

No we don't. I was considering removing it but then thinking about the pain of
renaming the ~400 occurences.

How about I remove it in a separate CL which is purely mechanical?

[Mattias Nissler (ping if slow), 2015-04-21 08:04:12]

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.h (right):

https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
components/pref_registry/pref_registry_syncable.h:44: UNSYNCABLE_PREF = 1 << 1,
On 2015/04/21 07:58:54, raymes wrote:
> On 2015/04/20 09:33:34, Mattias Nissler wrote:
> > Do we really need a separate flag value for this? I would have expected this
> to
> > map to NO_REGISTRATION_FLAG.
> 
> No we don't. I was considering removing it but then thinking about the pain of
> renaming the ~400 occurences.
> 
> How about I remove it in a separate CL which is purely mechanical?

We could also just declare UNSYNCABLE_PREF = 0 here and be done with it :)

[raymes, 2015-04-22 04:40:16]

On 2015/04/21 08:04:12, Mattias Nissler wrote:
>
https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
> File components/pref_registry/pref_registry_syncable.h (right):
> 
>
https://codereview.chromium.org/1096833003/diff/20001/components/pref_registr...
> components/pref_registry/pref_registry_syncable.h:44: UNSYNCABLE_PREF = 1 <<
1,
> On 2015/04/21 07:58:54, raymes wrote:
> > On 2015/04/20 09:33:34, Mattias Nissler wrote:
> > > Do we really need a separate flag value for this? I would have expected
this
> > to
> > > map to NO_REGISTRATION_FLAG.
> > 
> > No we don't. I was considering removing it but then thinking about the pain
of
> > renaming the ~400 occurences.
> > 
> > How about I remove it in a separate CL which is purely mechanical?
> 
> We could also just declare UNSYNCABLE_PREF = 0 here and be done with it :)

Done :) PTAL

I think I'll still try to remove UNSYNCABLE_PREF in a separate CL though.
Thanks!

[Mattias Nissler (ping if slow), 2015-04-22 07:06:34]

https://codereview.chromium.org/1096833003/diff/60001/base/prefs/pref_registry.h
File base/prefs/pref_registry.h (right):

https://codereview.chromium.org/1096833003/diff/60001/base/prefs/pref_registr...
base/prefs/pref_registry.h:48: const PrefRegistrationFlagsMap&
registration_flags() const {
OK, so you made the change to not store entries if flags == 0. Please also
replace this function with a GetRegistrationFlags(const std::string& pref)
function that returns the correct flags value regardless of whether the pref has
an entry in |registration_flags_|.

[raymes, 2015-04-23 05:21:03]

https://codereview.chromium.org/1096833003/diff/60001/base/prefs/pref_registry.h
File base/prefs/pref_registry.h (right):

https://codereview.chromium.org/1096833003/diff/60001/base/prefs/pref_registr...
base/prefs/pref_registry.h:48: const PrefRegistrationFlagsMap&
registration_flags() const {
On 2015/04/22 07:06:34, Mattias Nissler wrote:
> OK, so you made the change to not store entries if flags == 0. Please also
> replace this function with a GetRegistrationFlags(const std::string& pref)
> function that returns the correct flags value regardless of whether the pref
has
> an entry in |registration_flags_|.

Done.

[Mattias Nissler (ping if slow), 2015-04-23 07:37:46]

LGTM, thanks!

https://codereview.chromium.org/1096833003/diff/80001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.h (right):

https://codereview.chromium.org/1096833003/diff/80001/components/pref_registr...
components/pref_registry/pref_registry_syncable.h:41: // Note These must NOT
overlap with PrefRegistry::PrefRegistrationFlags.
nit: colon after "Note" or lower-case "These"

[raymes, 2015-04-24 01:01:07]

https://codereview.chromium.org/1096833003/diff/80001/components/pref_registr...
File components/pref_registry/pref_registry_syncable.h (right):

https://codereview.chromium.org/1096833003/diff/80001/components/pref_registr...
components/pref_registry/pref_registry_syncable.h:41: // Note These must NOT
overlap with PrefRegistry::PrefRegistrationFlags.
On 2015/04/23 07:37:46, Mattias Nissler wrote:
> nit: colon after "Note" or lower-case "These"

Done.

[raymes, 2015-04-24 01:01:18]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-04-24 01:01:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1096833003/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-04-24 01:02:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1096833003/100001

[commit-bot: I haz the power, 2015-04-24 01:06:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-24 01:06:08]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[raymes, 2015-04-24 01:36:29]

The CQ bit was checked by raymes@chromium.org

[raymes, 2015-04-24 01:36:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
mnissler@chromium.org
Link to the patchset: https://codereview.chromium.org/1096833003/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-04-24 01:36:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1096833003/160001

[commit-bot: I haz the power, 2015-04-24 02:45:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-04-24 02:46:21]

Patchset 9 (id:??) landed as
https://crrev.com/51b41a6f25281c3d7378eb21a69e4fcacc8d13f2
Cr-Commit-Position: refs/heads/master@{#326727}