[Sync] Make directory types registration explicit in ModelTypeRegistry

[Sync] Make directory types registration explicit in ModelTypeRegistry

The goal of this change is to make registration of directory types with
ModelTypeRegistry to be driven by individual data type controllers. Today set of
directory data types are registered with single call to
SetEnabledDirectoryTypes, they are derived from routing info passed through
sync manager and scheduler.

Since datatype registration will be performed as separate posts to sync thread
it is important to switch scheduler to configuration mode at the beginning of
configuration.

In this change I:
- Added Register/UnregisterDirectoryType to ModelTypeConnector and
  ModelTypeRegistry.
- Removed SetEnabledDirectoryTypes form ModelTypeRegistry as it is no longer
  needed.
- Added StartConfiguration to SyncEngine. This method needs to be called before
  RegisterDirectoryDataType are called so that normal sync cycle doesn't get
  executed on sync thread and doesn't include new datatypes before their
  services are ready to receive updates.
- Added BeforeLoadModels method to DataTypeController. This method is called
  right before LoadModels and allows directory datatypes to register with sync
  engine before initial data downloading begins.

BUG=647505
R=maxbogue@chromium.org

Review-Url: https://codereview.chromium.org/2641523004
Cr-Commit-Position: refs/heads/master@{#444846}
Committed: https://chromium.googlesource.com/chromium/src/+/1c7af89ea2d8e6e5e31be9d1f7bfa6c306584f5d

[pavely, 2017-01-18 00:47:23]

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

[pavely, 2017-01-18 00:48:47]

Description was changed from

==========
[Sync] Make directyr types registration explicit in ModelTypeRegistry

The goal of this change is to make registration of directory types with
ModelTypeRegistry to be driven by individual data type controllers. Today set of
directory data types are registered with single call to
SetEnabledDirectoryTypes, they are derived from routing info passed through
sync manager and scheduler.

Since datatype registration will be performed as separate posts to sync thread
it is important to switch scheduler to configuration mode at the beginning of
configuration.

In this change I:
- Added Register/UnregisterDirectoryType to ModelTypeConnector and
  ModelTypeRegistry.
- Removed SetEnabledDirectoryTypes form ModelTypeRegistry as it is no longer
  needed.
- Added StartConfiguration to SyncEngine. This method needs to be called before
  RegisterDirectoryDataType are called so that normal sync cycle doesn't get
  executed on sync thread and doesn't include new datatypes before their
  services are ready to receive updates.
- Added BeforeLoadModels method to DataTypeController. This method is called
  right before LoadModels and allows directory datatypes to register with sync
  engine before initial data downloading begins.

BUG=647505
R=maxbogue@chromium.org
==========

to

==========
[Sync] Make directory types registration explicit in ModelTypeRegistry

The goal of this change is to make registration of directory types with
ModelTypeRegistry to be driven by individual data type controllers. Today set of
directory data types are registered with single call to
SetEnabledDirectoryTypes, they are derived from routing info passed through
sync manager and scheduler.

Since datatype registration will be performed as separate posts to sync thread
it is important to switch scheduler to configuration mode at the beginning of
configuration.

In this change I:
- Added Register/UnregisterDirectoryType to ModelTypeConnector and
  ModelTypeRegistry.
- Removed SetEnabledDirectoryTypes form ModelTypeRegistry as it is no longer
  needed.
- Added StartConfiguration to SyncEngine. This method needs to be called before
  RegisterDirectoryDataType are called so that normal sync cycle doesn't get
  executed on sync thread and doesn't include new datatypes before their
  services are ready to receive updates.
- Added BeforeLoadModels method to DataTypeController. This method is called
  right before LoadModels and allows directory datatypes to register with sync
  engine before initial data downloading begins.

BUG=647505
R=maxbogue@chromium.org
==========

[commit-bot: I haz the power, 2017-01-18 00:48:52]

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-18 01:24:12]

Quick initial pass; I skipped all the test stuff, which I'll look at tomorrow.

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/glue...
File components/sync/driver/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/glue...
components/sync/driver/glue/sync_backend_host_core.cc:146: // update handlers
registerd in ModelTypeRegistry. Register them here.
registered

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/glue...
components/sync/driver/glue/sync_backend_host_core.cc:147: auto
model_type_connector = sync_manager_->GetModelTypeConnectorProxy();
I think you should probably just have an accessor for ModelTypeRegistry* (or
ModelTypeConnector*) on SyncManager. This is creating a whole new proxy object
and the RegisterDirectoryType call will be asynchronous because of it, neither
of which are necessary.

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_association_manager.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/mode...
components/sync/driver/model_association_manager.cc:172:
delegate_->OnSingleDataTypeWillStart(dtc->type());
Do you need to thread this call through DataTypeManagerImpl? It's just accessing
the DTC and calling BeforeLoadModels on it, and the ModelAssociationManager
could do that right here.

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine/mode...
File components/sync/engine/model_type_connector.h (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine/mode...
components/sync/engine/model_type_connector.h:17: // types through methods of
this interface consumer controlls which types will
controls

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/model_type_registry.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:173: DCHECK(group !=
GROUP_NON_BLOCKING);
DCHECK_NE(GROUP_NON_BLOCKING, group);

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:178: auto new_emitter =
base::MakeUnique<DirectoryTypeDebugInfoEmitter>(
I think owned_emitter would be clearer maybe? Or call this one emitter and the
raw pointer emitter_ptr, which is what we do in a lot of other places this
pattern is used.

https://codereview.chromium.org/2641523004/diff/1/components/sync/tools/sync_...
File components/sync/tools/sync_client.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/tools/sync_...
components/sync/tools/sync_client.cc:444:
model_type_connector->RegisterDirectoryType(it.Get(), GROUP_PASSIVE);
Same comment here as SBHC.

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

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

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

Dry run: This issue passed the CQ dry run.

[pavely, 2017-01-18 20:17:28]

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

[commit-bot: I haz the power, 2017-01-18 20:17:54]

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

[pavely, 2017-01-18 20:18:07]

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/glue...
File components/sync/driver/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/glue...
components/sync/driver/glue/sync_backend_host_core.cc:146: // update handlers
registerd in ModelTypeRegistry. Register them here.
On 2017/01/18 01:24:11, maxbogue wrote:
> registered

Done.

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/glue...
components/sync/driver/glue/sync_backend_host_core.cc:147: auto
model_type_connector = sync_manager_->GetModelTypeConnectorProxy();
On 2017/01/18 01:24:11, maxbogue wrote:
> I think you should probably just have an accessor for ModelTypeRegistry* (or
> ModelTypeConnector*) on SyncManager. This is creating a whole new proxy object
> and the RegisterDirectoryType call will be asynchronous because of it, neither
> of which are necessary.

Done.

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/mode...
File components/sync/driver/model_association_manager.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/driver/mode...
components/sync/driver/model_association_manager.cc:172:
delegate_->OnSingleDataTypeWillStart(dtc->type());
On 2017/01/18 01:24:11, maxbogue wrote:
> Do you need to thread this call through DataTypeManagerImpl? It's just
accessing
> the DTC and calling BeforeLoadModels on it, and the ModelAssociationManager
> could do that right here.

All the code that deals with registering types is in DTMI, I didn't want to
pollute MAM with just this one call. And to be honest I think LoadModels doesn't
belong in MAM either.

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine/mode...
File components/sync/engine/model_type_connector.h (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine/mode...
components/sync/engine/model_type_connector.h:17: // types through methods of
this interface consumer controlls which types will
On 2017/01/18 01:24:11, maxbogue wrote:
> controls

Done.

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine_impl...
File components/sync/engine_impl/model_type_registry.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:173: DCHECK(group !=
GROUP_NON_BLOCKING);
On 2017/01/18 01:24:11, maxbogue wrote:
> DCHECK_NE(GROUP_NON_BLOCKING, group);

Done.

https://codereview.chromium.org/2641523004/diff/1/components/sync/engine_impl...
components/sync/engine_impl/model_type_registry.cc:178: auto new_emitter =
base::MakeUnique<DirectoryTypeDebugInfoEmitter>(
On 2017/01/18 01:24:11, maxbogue wrote:
> I think owned_emitter would be clearer maybe? Or call this one emitter and the
> raw pointer emitter_ptr, which is what we do in a lot of other places this
> pattern is used.

Done.

https://codereview.chromium.org/2641523004/diff/1/components/sync/tools/sync_...
File components/sync/tools/sync_client.cc (right):

https://codereview.chromium.org/2641523004/diff/1/components/sync/tools/sync_...
components/sync/tools/sync_client.cc:444:
model_type_connector->RegisterDirectoryType(it.Get(), GROUP_PASSIVE);
On 2017/01/18 01:24:11, maxbogue wrote:
> Same comment here as SBHC.

Done.

[commit-bot: I haz the power, 2017-01-18 21:12:30]

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

[commit-bot: I haz the power, 2017-01-18 21:12:32]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2017-01-19 19:19:55]

lgtm! Comments are all little nits :)

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/data_type_controller.h:94: // Called right before
LoadModels. This method allows controller to register
s/controller/the type

or the model type. Registering controllers is a different thing that happens in
PSS.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.h:31: void
BeforeLoadModels(ModelTypeConfigurer* configurer) override;
I'd try to keep this in the same order as in DTC; right below
ShouldLoadModelBeforeConfigure. It's a little awkward because of the comments
but eh.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/model_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/model_type_controller.h:41: void
BeforeLoadModels(ModelTypeConfigurer* configurer) override;
Maintain order with DTC; put this right above LoadModels, here and in the .cc
file.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/model_type_controller_unittest.cc (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/model_type_controller_unittest.cc:99: void
UnregisterDirectoryDataType(ModelType type) override {
newline above for consistency?

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/proxy_data_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/proxy_data_type_controller.h:27: void
BeforeLoadModels(ModelTypeConfigurer* configurer) override;
Above LoadModels, here and in cc

https://codereview.chromium.org/2641523004/diff/20001/components/sync/engine/...
File components/sync/engine/sync_manager.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/engine/...
components/sync/engine/sync_manager.h:358: // ModelTypeConnectorproxy all calls
are executed synchronously, thus the
proxy -> Proxy

[pavely, 2017-01-19 20:27:00]

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

[commit-bot: I haz the power, 2017-01-19 20:27:20]

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

[pavely, 2017-01-19 20:27:39]

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/data_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/data_type_controller.h:94: // Called right before
LoadModels. This method allows controller to register
On 2017/01/19 19:19:54, maxbogue wrote:
> s/controller/the type
> 
> or the model type. Registering controllers is a different thing that happens
in
> PSS.

Controllers are the one performing registration, difference between directory
and USS registration is coded in controller. I changed comment to "allows
controller to register the type with sync engine".

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/directory_data_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/directory_data_type_controller.h:31: void
BeforeLoadModels(ModelTypeConfigurer* configurer) override;
On 2017/01/19 19:19:54, maxbogue wrote:
> I'd try to keep this in the same order as in DTC; right below
> ShouldLoadModelBeforeConfigure. It's a little awkward because of the comments
> but eh.

It is GetAllNodes and GetStatusCounters that are out of order. Moved them down
to their proper place.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/model_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/model_type_controller.h:41: void
BeforeLoadModels(ModelTypeConfigurer* configurer) override;
On 2017/01/19 19:19:54, maxbogue wrote:
> Maintain order with DTC; put this right above LoadModels, here and in the .cc
> file.

Done.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/model_type_controller_unittest.cc (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/model_type_controller_unittest.cc:99: void
UnregisterDirectoryDataType(ModelType type) override {
On 2017/01/19 19:19:54, maxbogue wrote:
> newline above for consistency?

Done.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
File components/sync/driver/proxy_data_type_controller.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/driver/...
components/sync/driver/proxy_data_type_controller.h:27: void
BeforeLoadModels(ModelTypeConfigurer* configurer) override;
On 2017/01/19 19:19:55, maxbogue wrote:
> Above LoadModels, here and in cc

Done.

https://codereview.chromium.org/2641523004/diff/20001/components/sync/engine/...
File components/sync/engine/sync_manager.h (right):

https://codereview.chromium.org/2641523004/diff/20001/components/sync/engine/...
components/sync/engine/sync_manager.h:358: // ModelTypeConnectorproxy all calls
are executed synchronously, thus the
On 2017/01/19 19:19:55, maxbogue wrote:
> proxy -> Proxy

Done.

[commit-bot: I haz the power, 2017-01-19 21:13:16]

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

[commit-bot: I haz the power, 2017-01-19 21:13:17]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-01-19 21:21:23]

The CQ bit was checked by pavely@chromium.org

[pavely, 2017-01-19 21:21:24]

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

[commit-bot: I haz the power, 2017-01-19 21:22:04]

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-19 21:28:28]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484860883266250,
"parent_rev": "d50af1abfb9aaf373226e9e4878ecefe000198ed", "commit_rev":
"1c7af89ea2d8e6e5e31be9d1f7bfa6c306584f5d"}

[commit-bot: I haz the power, 2017-01-19 21:28:59]

Description was changed from

==========
[Sync] Make directory types registration explicit in ModelTypeRegistry

The goal of this change is to make registration of directory types with
ModelTypeRegistry to be driven by individual data type controllers. Today set of
directory data types are registered with single call to
SetEnabledDirectoryTypes, they are derived from routing info passed through
sync manager and scheduler.

Since datatype registration will be performed as separate posts to sync thread
it is important to switch scheduler to configuration mode at the beginning of
configuration.

In this change I:
- Added Register/UnregisterDirectoryType to ModelTypeConnector and
  ModelTypeRegistry.
- Removed SetEnabledDirectoryTypes form ModelTypeRegistry as it is no longer
  needed.
- Added StartConfiguration to SyncEngine. This method needs to be called before
  RegisterDirectoryDataType are called so that normal sync cycle doesn't get
  executed on sync thread and doesn't include new datatypes before their
  services are ready to receive updates.
- Added BeforeLoadModels method to DataTypeController. This method is called
  right before LoadModels and allows directory datatypes to register with sync
  engine before initial data downloading begins.

BUG=647505
R=maxbogue@chromium.org
==========

to

==========
[Sync] Make directory types registration explicit in ModelTypeRegistry

The goal of this change is to make registration of directory types with
ModelTypeRegistry to be driven by individual data type controllers. Today set of
directory data types are registered with single call to
SetEnabledDirectoryTypes, they are derived from routing info passed through
sync manager and scheduler.

Since datatype registration will be performed as separate posts to sync thread
it is important to switch scheduler to configuration mode at the beginning of
configuration.

In this change I:
- Added Register/UnregisterDirectoryType to ModelTypeConnector and
  ModelTypeRegistry.
- Removed SetEnabledDirectoryTypes form ModelTypeRegistry as it is no longer
  needed.
- Added StartConfiguration to SyncEngine. This method needs to be called before
  RegisterDirectoryDataType are called so that normal sync cycle doesn't get
  executed on sync thread and doesn't include new datatypes before their
  services are ready to receive updates.
- Added BeforeLoadModels method to DataTypeController. This method is called
  right before LoadModels and allows directory datatypes to register with sync
  engine before initial data downloading begins.

BUG=647505
R=maxbogue@chromium.org

Review-Url: https://codereview.chromium.org/2641523004
Cr-Commit-Position: refs/heads/master@{#444846}
Committed:
https://chromium.googlesource.com/chromium/src/+/1c7af89ea2d8e6e5e31be9d1f7bf...
==========

[commit-bot: I haz the power, 2017-01-19 21:29:00]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1c7af89ea2d8e6e5e31be9d1f7bf...

[pavely, 2017-01-20 18:35:04]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2644373003/ by pavely@chromium.org.

The reason for reverting is: This change causes crashes in sync:
http://crbug.com/682998
.