[Sync] Move ConfigureDataTypes logic into DataTypeManagerImpl.

[Sync] Move ConfigureDataTypes logic into DataTypeManagerImpl.

This is good because it sets the stage for refactoring this logic and
testing it better, but it's bad because it reveals the full extent of
our sins with respect to the current state of the code.

The primary functional change is that DTMI tracks the set of types it
thinks the engine knows are... downloaded? Configured? Whatever the
types in SyncBackendRegistrar's routing info represented before. Now it
has a copy on the UI thread that it works with, and SBR's
ConfigureDataTypes is no longer called from the UI thread.

BUG=669967, 663125
Review-Url: https://codereview.chromium.org/2563423005
Cr-Commit-Position: refs/heads/master@{#442370}
Committed: https://chromium.googlesource.com/chromium/src/+/82712717bb8f87ad7cc66e831d61d5fcb5352053

[maxbogue, 2016-12-16 05:36:16]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 05:36:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-16 05:37:07]

Description was changed from

==========
[Sync] Move ConfigureDataTypes logic into DataTypeManagerImpl.

Work in progress - DO NOT SUBMIT.

BUG=669967,663125
==========

to

==========
[Sync] Move ConfigureDataTypes logic into DataTypeManagerImpl.

This is good because it sets the stage for refactoring this logic and
testing it better, but it's bad because it reveals the full extent of
our sins with respect to the current state of the code.

The primary functional change is that DTMI tracks the set of types it
thinks the engine knows are... downloaded? Configured? Whatever the
types in SyncBackendRegistrar's routing info represented before. Now it
has a copy on the UI thread that it works with, and SBR's
ConfigureDataTypes is no longer called from the UI thread.

BUG=669967,663125
==========

[commit-bot: I haz the power, 2016-12-16 08:44:00]

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

[commit-bot: I haz the power, 2016-12-16 08:44:01]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2016-12-16 18:31:04]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 18:31:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-16 18:33:11]

maxbogue@chromium.org changed reviewers:
+ pavely@chromium.org, skym@chromium.org

[maxbogue, 2016-12-16 18:33:12]

Sky, Pavel, PTAL! I'm not super happy with the unittest changes but I want to
see if we can make the logic clearer (i.e. make sure it actually works right)
before adding more coverage.

[commit-bot: I haz the power, 2016-12-16 21:35:21]

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

[commit-bot: I haz the power, 2016-12-16 21:35:22]

Dry run: This issue passed the CQ dry run.

[skym, 2016-12-19 18:54:17]

lgtm

https://codereview.chromium.org/2563423005/diff/40001/components/sync/driver/...
File components/sync/driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/2563423005/diff/40001/components/sync/driver/...
components/sync/driver/data_type_manager_impl.cc:171: ModelTypeSet
DataTypeManagerImpl::GetDataTypesInState(
Should DataTypeConfigStateMap be a class that has these instanced methods?

[maxbogue, 2017-01-04 18:38:22]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 18:38:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 19:36:25]

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

[commit-bot: I haz the power, 2017-01-04 19:36:26]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2017-01-04 21:41:30]

Pavel, PTAL! This should be ready now (updated for your patch regarding clean
types).

[maxbogue, 2017-01-04 21:46:26]

https://codereview.chromium.org/2563423005/diff/40001/components/sync/driver/...
File components/sync/driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/2563423005/diff/40001/components/sync/driver/...
components/sync/driver/data_type_manager_impl.cc:171: ModelTypeSet
DataTypeManagerImpl::GetDataTypesInState(
I don't think that's worth it for now for something that's just internal. This
code will probably keep changing in other ways soon anyways.

[skym, 2017-01-05 00:42:11]

Still looks good.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
File components/sync/driver/data_type_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl_unittest.cc:29: return result;
Could this just return Union(ControlTypes(), ModelTypeSet(type)); ?

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl_unittest.cc:32: ModelTypeSet
AddControlTypesTo(ModelTypeSet types) {
While you're in here, this should use Union() as well.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl_unittest.cc:86: 
Fighting the good fight!

[pavely, 2017-01-05 21:32:19]

I left couple of comments in the code. In addition we discussed two things that
current implementation doesn't handle:
- filtered types from SyncBackendRegistrar (passwords and history)
- initial_sync_done from USS datatypes.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
File components/sync/driver/data_type_manager_impl.h (right):

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl.h:81: ModelTypeSet engine_types_;
This set represents types which don't need to be downloaded as part of
configuration. Sync data for these types is locally available. Alternative names
could be:
- types_with_local_data_
- locally_available_types_

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl.h:126: ModelTypeSet
ConfigureDataTypes();
Name "ConfigureDataTypes" can be confused with Configure() and ConfigureImpl().
At the same time this function performs two tasks:
- Translate from local state to configuration parameters
- Invoke configurer

Maybe you could refactor it into PrepareConfigurationParameters which populates
parameters and then call configurer directly from StartNextDownload.

[maxbogue, 2017-01-06 18:02:44]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 18:03:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2017-01-06 18:55:58]

PTAL! I've made another CL this one depends on for filtering out passwords and
history, and I've used a callback for marking whether USS types are downloaded
in this CL.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
File components/sync/driver/data_type_manager_impl.h (right):

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl.h:81: ModelTypeSet engine_types_;
On 2017/01/05 21:32:19, pavely wrote:
> This set represents types which don't need to be downloaded as part of
> configuration. Sync data for these types is locally available. Alternative
names
> could be:
> - types_with_local_data_
> - locally_available_types_

I've gone with downloaded_types_, which I think is the most concise and clear.
It's easy to change though, so let me know if you disagree!

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl.h:126: ModelTypeSet
ConfigureDataTypes();
On 2017/01/05 21:32:19, pavely wrote:
> Name "ConfigureDataTypes" can be confused with Configure() and
ConfigureImpl().
> At the same time this function performs two tasks:
> - Translate from local state to configuration parameters
> - Invoke configurer
> 
> Maybe you could refactor it into PrepareConfigurationParameters which
populates
> parameters and then call configurer directly from StartNextDownload.

Done.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
File components/sync/driver/data_type_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl_unittest.cc:29: return result;
On 2017/01/05 00:42:11, skym wrote:
> Could this just return Union(ControlTypes(), ModelTypeSet(type)); ?

Done.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl_unittest.cc:32: ModelTypeSet
AddControlTypesTo(ModelTypeSet types) {
On 2017/01/05 00:42:11, skym wrote:
> While you're in here, this should use Union() as well.

Done.

https://codereview.chromium.org/2563423005/diff/60001/components/sync/driver/...
components/sync/driver/data_type_manager_impl_unittest.cc:86: 
On 2017/01/05 00:42:11, skym wrote:
> Fighting the good fight!

\o/

[commit-bot: I haz the power, 2017-01-06 19:18:17]

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

[commit-bot: I haz the power, 2017-01-06 19:18:18]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-01-09 19:35:06]

lgtm

[maxbogue, 2017-01-09 19:50:30]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2017-01-09 19:50:31]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2563423005/#ps80001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-01-09 19:50:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 22:22:12]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483991430885170,
"parent_rev": "8890be37e55775f3fae49e71883c81a7c4ddd22a", "commit_rev":
"82712717bb8f87ad7cc66e831d61d5fcb5352053"}

[commit-bot: I haz the power, 2017-01-09 22:22:39]

Description was changed from

==========
[Sync] Move ConfigureDataTypes logic into DataTypeManagerImpl.

This is good because it sets the stage for refactoring this logic and
testing it better, but it's bad because it reveals the full extent of
our sins with respect to the current state of the code.

The primary functional change is that DTMI tracks the set of types it
thinks the engine knows are... downloaded? Configured? Whatever the
types in SyncBackendRegistrar's routing info represented before. Now it
has a copy on the UI thread that it works with, and SBR's
ConfigureDataTypes is no longer called from the UI thread.

BUG=669967,663125
==========

to

==========
[Sync] Move ConfigureDataTypes logic into DataTypeManagerImpl.

This is good because it sets the stage for refactoring this logic and
testing it better, but it's bad because it reveals the full extent of
our sins with respect to the current state of the code.

The primary functional change is that DTMI tracks the set of types it
thinks the engine knows are... downloaded? Configured? Whatever the
types in SyncBackendRegistrar's routing info represented before. Now it
has a copy on the UI thread that it works with, and SBR's
ConfigureDataTypes is no longer called from the UI thread.

BUG=669967,663125

Review-Url: https://codereview.chromium.org/2563423005
Cr-Commit-Position: refs/heads/master@{#442370}
Committed:
https://chromium.googlesource.com/chromium/src/+/82712717bb8f87ad7cc66e831d61...
==========

[commit-bot: I haz the power, 2017-01-09 22:22:40]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/82712717bb8f87ad7cc66e831d61...