Add a declarative way in c/b/s/sync_prefs.cc to say which data types have prefs

Add a declarative way in c/b/s/sync_prefs.cc to say which data types have prefs
whose values are determined by others (e.g. APP_NOTIFICATIONS on APPS).
Make the hand-written code use the system, and add {APP,EXTENSION}_SETTINGS.

R=akalin@chromium.org
TBR=estade@chromium.org
BUG=114974
TEST=unit_tests --gtest_filter=*Sync*


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

[akalin, 2012-02-29 05:33:03]

thanks for doing this!  few comments

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
File chrome/browser/sync/sync_prefs.cc (right):

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.cc:24: RegisterDeterminedTypes();
this doesn't need to be in the if statement

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.cc:142: // Remove any data types whose prefs
depend on other types (will be added
with ApplyTypeDependencies as described in the comment below, this can just be:

return ApplyTypeDependencies(registered_types, GetRawPreferredTypes());

where GetRawPreferredTypes() just loops over ModelTypeSet::All() and checks
GetDataTypePreferred() for each one and returns the result.

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.cc:182: i != types_determined_by_.end(); ++i) {
this is almost the same logic as in GetPreferredDataTypes; we probably should
consolidate the code (I prefer the GetPreferredDataTypes version).

How about a function like:

// With the given set of registered types, applies type dependencies to |types|
and returns the result.  |types| should be a subset of |registered_types|. The
return value is not necessarily a superset of |types|, but it's a subset of
|registered_types|.
ModelTypeSet ApplyTypeDependencies(ModelTypeSet registered_types, ModelTypeSet
types) {
   ...
}

Then this can be:

SetRawPreferredTypes(ApplyTypeDependencies(registered_types, preferred_types));

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.cc:441: model_set.Put(syncable::APP_SETTINGS);
don't add new types -- see comment above definition of model_set.

in fact, probably best to leave in original order.

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
File chrome/browser/sync/sync_prefs.h (right):

http://codereview.chromium.org/9500005/diff/1005/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.h:144: TypesDeterminedByMap;
These names are kinda awkward.  How about:

typedef ... TypeDependencyMap;
TypeDependencyMap type_dependencies_;

[not at google - send to devlin, 2012-03-01 03:14:40]

https://chromiumcodereview.appspot.com/9500005/diff/1005/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9500005/diff/1005/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:24: RegisterDeterminedTypes();
On 2012/02/29 05:33:03, akalin wrote:
> this doesn't need to be in the if statement

Done.

https://chromiumcodereview.appspot.com/9500005/diff/1005/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:441: model_set.Put(syncable::APP_SETTINGS);
On 2012/02/29 05:33:03, akalin wrote:
> don't add new types -- see comment above definition of model_set.
> 
> in fact, probably best to leave in original order.

Oh, right. I was wondering what that comment meant. It's pretty vague i.e. not
clear what model_set actually does.

The comment should say like "Do not add new data types to this set. We will
start prompting people..."

And then "model_set" should be called, like, whatever it's supposed to do.

Anyway, change reverted (though I prefer it sorted...)

https://chromiumcodereview.appspot.com/9500005/diff/1005/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.h (right):

https://chromiumcodereview.appspot.com/9500005/diff/1005/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.h:144: TypesDeterminedByMap;
On 2012/02/29 05:33:03, akalin wrote:
> These names are kinda awkward.  How about:
> 
> typedef ... TypeDependencyMap;
> TypeDependencyMap type_dependencies_;

I avoided using the term "dependency" because it gives the wrong connotation in
the context of software; I think of "A is a dependency of B" as meaning that "A
must be in a valid state before we can think about B", but that's not what this
is. Maybe "DependentTypesMap"/"dependent_types_" would be more accurate?

Or maybe this is thinking about the wrong way. Chatting to a colleague, perhaps
a more sensible model would be to think of it as "pref groups" such that the
value of one pref determines the values of a group of prefs. So this would be

// Groups of prefs that always have the same value as a master pref.
// For example, the APPS group has {APP_NOTIFICATIONS, APP_SETTINGS}
// (as well as APPS, but that is implied), so
//   pref_groups_[syncable::APPS] = { syncable::APP_NOTIFICATIONS,
syncable::APP_SETTINGS}
//   pref_groups_[syncable::EXTENSIONS = { syncable::EXTENSION_SETTINGS }
// etc.
typedef std::map<syncable::ModelType, ModelTypeSet> PrefGroupsMap;
PrefGroupsMap pref_groups_;

It'd also be slightly more efficient that way.

In the meantime though, pending your opinion about this, I'll go with the
original plan and use DependentTypesMap/dependent_types_.

[not at google - send to devlin, 2012-03-01 03:16:06]

(Btw I didn't add {Get,Set}Raw... because they would have only been used in one
spot, seemed unnecessary)

[akalin, 2012-03-01 07:34:52]

> I avoided using the term "dependency" because it gives the wrong connotation
in
> the context of software; I think of "A is a dependency of B" as meaning that
"A
> must be in a valid state before we can think about B", but that's not what
this
> is. Maybe "DependentTypesMap"/"dependent_types_" would be more accurate?
> 
> Or maybe this is thinking about the wrong way. Chatting to a colleague,
perhaps
> a more sensible model would be to think of it as "pref groups" such that the
> value of one pref determines the values of a group of prefs. So this would be
> 
> // Groups of prefs that always have the same value as a master pref.
> // For example, the APPS group has {APP_NOTIFICATIONS, APP_SETTINGS}
> // (as well as APPS, but that is implied), so
> //   pref_groups_[syncable::APPS] = { syncable::APP_NOTIFICATIONS,
> syncable::APP_SETTINGS}
> //   pref_groups_[syncable::EXTENSIONS = { syncable::EXTENSION_SETTINGS }
> // etc.
> typedef std::map<syncable::ModelType, ModelTypeSet> PrefGroupsMap;
> PrefGroupsMap pref_groups_;
> 
> It'd also be slightly more efficient that way.
> 
> In the meantime though, pending your opinion about this, I'll go with the
> original plan and use DependentTypesMap/dependent_types_.

The "pref group" way would have been how I would have originally done it,
actually.  I kinda like it.  The typedef/var name above looks good.

[akalin, 2012-03-01 07:40:59]

few more comments, plus changing to using PrefGroups

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
File chrome/browser/sync/sync_prefs.cc (left):

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.cc:129: if
(pref_service_->HasPrefPath(prefs::kSavingBrowserHistoryDisabled) &&
Hmm...I'm a bit bothered by the fact that we filter registered types here but
not in the other function.  But I guess it's okay for now; don't ahve to deal
with it in this CL.

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
File chrome/browser/sync/sync_prefs.cc (right):

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.cc:463: const syncable::ModelTypeSet&
registered_types,
don't use refs for ModelTypeSet (they're <64 bits)

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.cc:466: syncable::ModelTypeSet
with_dependent_types(types);
I think we still want to first remove the types in types that depend on other
types (e.g., APP_NOTIFICATIONS, AUTOFILL_PROFILES, etc.) before adding the
dependent types.

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
File chrome/browser/sync/sync_prefs.h (right):

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.h:121: void RegisterDependentTypes();
I guess this'll be RegisterPrefGroups

http://codereview.chromium.org/9500005/diff/10001/chrome/browser/sync/sync_pr...
chrome/browser/sync/sync_prefs.h:129: // Returns a ModelTypeSet based on |types|
augmented with any dependent types
augmented with any dependent types ... ->
expanded to include pref groups (see |pref_groups_|

[not at google - send to devlin, 2012-03-01 23:39:36]

https://chromiumcodereview.appspot.com/9500005/diff/10001/chrome/browser/sync...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9500005/diff/10001/chrome/browser/sync...
chrome/browser/sync/sync_prefs.cc:463: const syncable::ModelTypeSet&
registered_types,
On 2012/03/01 07:40:59, akalin wrote:
> don't use refs for ModelTypeSet (they're <64 bits)

Done.

https://chromiumcodereview.appspot.com/9500005/diff/10001/chrome/browser/sync...
chrome/browser/sync/sync_prefs.cc:466: syncable::ModelTypeSet
with_dependent_types(types);
On 2012/03/01 07:40:59, akalin wrote:
> I think we still want to first remove the types in types that depend on other
> types (e.g., APP_NOTIFICATIONS, AUTOFILL_PROFILES, etc.) before adding the
> dependent types.

Done (the equivalent of).

https://chromiumcodereview.appspot.com/9500005/diff/10001/chrome/browser/sync...
File chrome/browser/sync/sync_prefs.h (right):

https://chromiumcodereview.appspot.com/9500005/diff/10001/chrome/browser/sync...
chrome/browser/sync/sync_prefs.h:121: void RegisterDependentTypes();
On 2012/03/01 07:40:59, akalin wrote:
> I guess this'll be RegisterPrefGroups

Done.

https://chromiumcodereview.appspot.com/9500005/diff/10001/chrome/browser/sync...
chrome/browser/sync/sync_prefs.h:129: // Returns a ModelTypeSet based on |types|
augmented with any dependent types
On 2012/03/01 07:40:59, akalin wrote:
> augmented with any dependent types ... ->
> expanded to include pref groups (see |pref_groups_|

Done.

[akalin, 2012-03-01 23:53:43]

LGTM thanks for doing this

[not at google - send to devlin, 2012-03-01 23:54:31]

No probs, thanks for the review.

[commit-bot: I haz the power, 2012-03-01 23:54:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9500005/19001

[commit-bot: I haz the power, 2012-03-01 23:54:59]

Presubmit check for 9500005-19001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/ui/webui/sync_setup_handler.cc

Presubmit checks took 1.1s to calculate.

[not at google - send to devlin, 2012-03-01 23:57:27]

@estade need lgtm from webui owners, and you're the lucky one who I just noticed
chatting on IRC :)

[commit-bot: I haz the power, 2012-03-02 03:06:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9500005/19001

[commit-bot: I haz the power, 2012-03-02 16:08:34]

Change committed as 124658