[Sync] Add CoreTypes support

[Sync] Add CoreTypes support

Previously we manually added "always enable" types from within the DataTypeManager.
Now the notion of types to always enable is pulled into its own construct, CoreTypes
(and it's sibling PriorityCoreTypes). Tests have been updated to exercise this, and
SyncedNotifications has been added as the first Core type.

BUG=245762
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212232

[Nicolas Zea, 2013-07-15 18:24:16]

PTAL

[tim (not reviewing), 2013-07-15 18:36:17]

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
chrome/browser/sync/glue/data_type_manager_impl.cc:84:
desired_types.Put(syncer::SYNCED_NOTIFICATIONS);
We don't enable SYNCED_NOTIFICATIONS on Android, right?

It'd be nice to have a formal notion (and Configure() contract) of core types /
"types that aren't profile sync that we always want to sync", like what is
discussed in the Core Services doc, but this looks good for now.  +rsimha FYI.

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (right):

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:47:
result.Put(syncer::SYNCED_NOTIFICATIONS);
Why does this need to be priority?  I don't see it that way in the production
code either?

[Nicolas Zea, 2013-07-15 22:48:42]

PTAL

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
chrome/browser/sync/glue/data_type_manager_impl.cc:84:
desired_types.Put(syncer::SYNCED_NOTIFICATIONS);
On 2013/07/15 18:36:17, timsteele wrote:
> We don't enable SYNCED_NOTIFICATIONS on Android, right?
> 
> It'd be nice to have a formal notion (and Configure() contract) of core types
/
> "types that aren't profile sync that we always want to sync", like what is
> discussed in the Core Services doc, but this looks good for now.  +rsimha FYI.

I've gone ahead and created a new notion of CoreTypes and PriorityCoreTypes to
help clean things up. And yeah, on other platforms there won't be a DTC, so the
type will get filtered out by the sync backend registrar.

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (right):

https://codereview.chromium.org/19227004/diff/1/chrome/browser/sync/glue/data...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:47:
result.Put(syncer::SYNCED_NOTIFICATIONS);
On 2013/07/15 18:36:17, timsteele wrote:
> Why does this need to be priority?  I don't see it that way in the production
> code either?

This was just to get the tests to pass because they were retrofitted to handle
priority types by making the tested types all be priority. I've gone ahead and
updated all the tests to more accurately reflect how priority types are used,
and to handle the new concept of core types.

[tim (not reviewing), 2013-07-16 16:54:54]

LGTM with a few nits / questions.

https://codereview.chromium.org/19227004/diff/12001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (right):

https://codereview.chromium.org/19227004/diff/12001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:59:
syncer::ModelTypeSet AddCoreTypesTo(syncer::ModelTypeSet types) {
I think I might just prefer CoreTypes().PutAll(ModelTypeSet(BOOKMARKS, APPS)) 

17 vs 14 characters with 'using' decl, not a huge difference, and it's nice to
see what's actually happening.

https://codereview.chromium.org/19227004/diff/12001/sync/internal_api/public/...
File sync/internal_api/public/base/model_type.h (right):

https://codereview.chromium.org/19227004/diff/12001/sync/internal_api/public/...
sync/internal_api/public/base/model_type.h:216: SYNC_EXPORT ModelTypeSet
CoreTypes();
I wonder if the "Core" distinction belongs in sync/ vs chrome/browser/sync...
fine for now, maybe Raghu can think about it.

https://codereview.chromium.org/19227004/diff/12001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/19227004/diff/12001/sync/syncable/model_type....
sync/syncable/model_type.cc:410: result.Put(MANAGED_USERS);
MANAGED_USER_SETTINGS fits the core type definition, but possibly not
priority...

https://codereview.chromium.org/19227004/diff/12001/sync/syncable/model_type....
sync/syncable/model_type.cc:414: ModelTypeSet CoreTypes() {
Should this include PriorityCoreTypes?  What if I want 'all' CoreTypes (which we
do in DTM)?

[Nicolas Zea, 2013-07-17 21:05:36]

Comments addressed, commitbot here I come!

https://codereview.chromium.org/19227004/diff/12001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/data_type_manager_impl_unittest.cc (right):

https://codereview.chromium.org/19227004/diff/12001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/data_type_manager_impl_unittest.cc:59:
syncer::ModelTypeSet AddCoreTypesTo(syncer::ModelTypeSet types) {
On 2013/07/16 16:54:55, timsteele wrote:
> I think I might just prefer CoreTypes().PutAll(ModelTypeSet(BOOKMARKS, APPS)) 
> 
> 17 vs 14 characters with 'using' decl, not a huge difference, and it's nice to
> see what's actually happening.

The issue is that PutAll doesn't return a ModelTypeSet, so you wind up needing
bunches of temp variables to hold and pass the result. This is useful for doing
stuff like set_expected_configure_types(AddCoreTypesTo(...)).

Additionally, now that we're using CoreTypes() to mean all managed types, this
also includes a difference now.

https://codereview.chromium.org/19227004/diff/12001/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

https://codereview.chromium.org/19227004/diff/12001/sync/syncable/model_type....
sync/syncable/model_type.cc:410: result.Put(MANAGED_USERS);
On 2013/07/16 16:54:55, timsteele wrote:
> MANAGED_USER_SETTINGS fits the core type definition, but possibly not
> priority...

I don't think it's a core type either? We weren't configuring with it previously
at least.

https://codereview.chromium.org/19227004/diff/12001/sync/syncable/model_type....
sync/syncable/model_type.cc:414: ModelTypeSet CoreTypes() {
On 2013/07/16 16:54:55, timsteele wrote:
> Should this include PriorityCoreTypes?  What if I want 'all' CoreTypes (which
we
> do in DTM)?

Yeah, I suppose CoreTypes() makes more sense as the union. Done.

[commit-bot: I haz the power, 2013-07-17 21:07:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/19227004/22001

[commit-bot: I haz the power, 2013-07-18 03:53:25]

Change committed as 212232