[Sync] Register USS types with ModelTypeRegistry before download

[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713
Committed: https://crrev.com/7f2ee45b59370b511a22ba5f7be2c366c51e572a
Cr-Commit-Position: refs/heads/master@{#385257}

[pavely, 2016-04-01 18:33:19]

Description was changed from

==========
[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713
==========

to

==========
[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713
==========

[pavely, 2016-04-01 18:59:49]

Patchset #2 (id:20001) has been deleted

[pavely, 2016-04-01 19:00:39]

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

[pavely, 2016-04-01 19:00:39]

pavely@chromium.org changed required reviewers:
+ zea@chromium.org

[skym, 2016-04-01 21:58:22]

lgtm

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/data...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/data...
components/sync_driver/data_type_manager_impl.cc:180: continue;
Why bother with continue? Why not just
  
  if (dtc_iter != controllers_->end()) {
    dtc_iter->second_>RegisterWithBackend(configurer_);
  }

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
File components/sync_driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
components/sync_driver/non_blocking_data_type_controller.cc:160:
DCHECK(!activation_context_);
Can you add a comment to ActivateDataType explaining that RegisterWithBackend is
doing the activating?

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:34: GROUP_PASSIVE = 0,   //
Models that are just "passively" being synced; e.g.
Can you update to make it clear that non-blocking types are never passive now?

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:45: GROUP_NON_BLOCKING,  //
Models that correspond to non-blocking types.
So as we migrate to USS, we'll be slowly switching everyone from their old
groups into GROUP_NON_BLOCKING?

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:46: MODEL_SAFE_GROUP_COUNT,
Doesn't look like this value is ever used.

[maxbogue, 2016-04-01 22:43:14]

lgtm

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
components/sync_driver/non_blocking_data_type_controller.h:53: // Registers
non-blocking data type with sync backend. In the process
In the process the

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
components/sync_driver/non_blocking_data_type_controller.h:54: // activation
context is passed to ModelTypeRegistry, ModelTypeWorker gets
"ModelTypeRegistry, ModelTypeWorker" -> "ModelTypeRegistry, where
ModelTypeWorker"

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (left):

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:298: scoped_ptr<syncer_v2::SyncContext>
sync_context_;
Good catch!

[pavely, 2016-04-04 17:57:16]

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/data...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/data...
components/sync_driver/data_type_manager_impl.cc:180: continue;
On 2016/04/01 21:58:21, skym wrote:
> Why bother with continue? Why not just
>   
>   if (dtc_iter != controllers_->end()) {
>     dtc_iter->second_>RegisterWithBackend(configurer_);
>   }

Sorry, work-in-progress version had more code after continue. Done.

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
File components/sync_driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
components/sync_driver/non_blocking_data_type_controller.cc:160:
DCHECK(!activation_context_);
On 2016/04/01 21:58:21, skym wrote:
> Can you add a comment to ActivateDataType explaining that RegisterWithBackend
is
> doing the activating?

Done.

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
components/sync_driver/non_blocking_data_type_controller.h:53: // Registers
non-blocking data type with sync backend. In the process
On 2016/04/01 22:43:14, maxbogue wrote:
> In the process the

Done.

https://codereview.chromium.org/1848793006/diff/1/components/sync_driver/non_...
components/sync_driver/non_blocking_data_type_controller.h:54: // activation
context is passed to ModelTypeRegistry, ModelTypeWorker gets
On 2016/04/01 22:43:14, maxbogue wrote:
> "ModelTypeRegistry, ModelTypeWorker" -> "ModelTypeRegistry, where
> ModelTypeWorker"

Done.

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:34: GROUP_PASSIVE = 0,   //
Models that are just "passively" being synced; e.g.
On 2016/04/01 21:58:22, skym wrote:
> Can you update to make it clear that non-blocking types are never passive now?

Done.

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:45: GROUP_NON_BLOCKING,  //
Models that correspond to non-blocking types.
On 2016/04/01 21:58:21, skym wrote:
> So as we migrate to USS, we'll be slowly switching everyone from their old
> groups into GROUP_NON_BLOCKING?

Yes. As we switch data type controllers to be derived from
NonBlockingDataTypeController they will be put into GROUP_NON_BLOCKING group by
SyncBackendRegistrar.

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/public/en...
sync/internal_api/public/engine/model_safe_worker.h:46: MODEL_SAFE_GROUP_COUNT,
On 2016/04/01 21:58:22, skym wrote:
> Doesn't look like this value is ever used.

Done.

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/sync_mana...
File sync/internal_api/sync_manager_impl.h (left):

https://codereview.chromium.org/1848793006/diff/1/sync/internal_api/sync_mana...
sync/internal_api/sync_manager_impl.h:298: scoped_ptr<syncer_v2::SyncContext>
sync_context_;
On 2016/04/01 22:43:14, maxbogue wrote:
> Good catch!

Acknowledged.

[maxbogue, 2016-04-04 18:08:14]

Still lgtm, just some tiny nits.

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.cc (left):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.cc:284: default:
Might be good to add the new GROUP_NON_BLOCKING here and remove default. That
way if another group gets added the compiler will make sure this switch also
gets updated.

https://codereview.chromium.org/1848793006/diff/40001/sync/internal_api/publi...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/1848793006/diff/40001/sync/internal_api/publi...
sync/internal_api/public/engine/model_safe_worker.h:46: // models always stay in
GROUP_NON_BLOCKING, changest are
s/, changest/; changes/

[Nicolas Zea, 2016-04-04 20:25:43]

lgtm

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/data_type_controller.h (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/data_type_controller.h:101: // Registers with sync
backend if needed. This function is called by
nit: explain why you would call this?

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:178: const auto dtc_iter =
controllers_->find(type_iter.Get());
nit: auto&?

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.cc (left):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.cc:284: default:
On 2016/04/04 18:08:14, maxbogue wrote:
> Might be good to add the new GROUP_NON_BLOCKING here and remove default. That
> way if another group gets added the compiler will make sure this switch also
> gets updated.

Agreed. I prefer to avoid using default when there's an enum if possible.

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.cc (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.cc:56: routing_info_[type] ==
syncer::GROUP_NON_BLOCKING);
is it valid to register a type more than once? Maybe get rid of second
condition?

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.h (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.h:72: // GROUP_NON_BLOCKING
initial model safe group and would be treated
nit: s/would/will

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.h:72: // GROUP_NON_BLOCKING
initial model safe group and would be treated
nit: Maybe also make clear that being in GROUP_NON_BLOCKING will never change
(i.e. it's not just "initial")

https://codereview.chromium.org/1848793006/diff/40001/sync/sessions/model_typ...
File sync/sessions/model_type_registry_unittest.cc (right):

https://codereview.chromium.org/1848793006/diff/40001/sync/sessions/model_typ...
sync/sessions/model_type_registry_unittest.cc:115:
EXPECT_TRUE(update_handler_map->find(APPS) == update_handler_map->end());
nit: EXPECT_EQ?

[pavely, 2016-04-05 18:21:45]

The CQ bit was checked by pavely@chromium.org

[pavely, 2016-04-05 18:21:45]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org,
zea@chromium.org, maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/1848793006/#ps60001
(title: "Addressed comments from Max and Nicolas.")

[pavely, 2016-04-05 18:21:52]

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/data_type_controller.h (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/data_type_controller.h:101: // Registers with sync
backend if needed. This function is called by
On 2016/04/04 20:25:42, Nicolas Zea wrote:
> nit: explain why you would call this?

Done.

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:178: const auto dtc_iter =
controllers_->find(type_iter.Get());
On 2016/04/04 20:25:42, Nicolas Zea wrote:
> nit: auto&?
You are right. go/totw:107.

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.cc (left):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.cc:284: default:
On 2016/04/04 18:08:14, maxbogue wrote:
> Might be good to add the new GROUP_NON_BLOCKING here and remove default. That
> way if another group gets added the compiler will make sure this switch also
> gets updated.

Done.

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.cc (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.cc:56: routing_info_[type] ==
syncer::GROUP_NON_BLOCKING);
On 2016/04/04 20:25:42, Nicolas Zea wrote:
> is it valid to register a type more than once? Maybe get rid of second
> condition?

It is valid. Data type could be enabled, then disabled, then enabled again.
REgisterNonBlockingType will be called both times type is enabled. I'll think
through this scenario when I'm working on support for disabling USS types.

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
File components/sync_driver/glue/sync_backend_registrar.h (right):

https://codereview.chromium.org/1848793006/diff/40001/components/sync_driver/...
components/sync_driver/glue/sync_backend_registrar.h:72: // GROUP_NON_BLOCKING
initial model safe group and would be treated
On 2016/04/04 20:25:43, Nicolas Zea wrote:
> nit: Maybe also make clear that being in GROUP_NON_BLOCKING will never change
> (i.e. it's not just "initial")

Done.

https://codereview.chromium.org/1848793006/diff/40001/sync/internal_api/publi...
File sync/internal_api/public/engine/model_safe_worker.h (right):

https://codereview.chromium.org/1848793006/diff/40001/sync/internal_api/publi...
sync/internal_api/public/engine/model_safe_worker.h:46: // models always stay in
GROUP_NON_BLOCKING, changest are
On 2016/04/04 18:08:14, maxbogue wrote:
> s/, changest/; changes/

Done. Thanks.

https://codereview.chromium.org/1848793006/diff/40001/sync/sessions/model_typ...
File sync/sessions/model_type_registry_unittest.cc (right):

https://codereview.chromium.org/1848793006/diff/40001/sync/sessions/model_typ...
sync/sessions/model_type_registry_unittest.cc:115:
EXPECT_TRUE(update_handler_map->find(APPS) == update_handler_map->end());
On 2016/04/04 20:25:43, Nicolas Zea wrote:
> nit: EXPECT_EQ?

I think the benefit of EXPECT_EQ is that it prints values of expressions for
both sides: value of ABC (which is 123) doesn't match value of XYZ (which is
789).

In this case value of iterator is probably some hex number which doesn't  make
sense, important part is whether it equals map::end() or not.

[commit-bot: I haz the power, 2016-04-05 18:22:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848793006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848793006/60001

[commit-bot: I haz the power, 2016-04-05 18:35:40]

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

[commit-bot: I haz the power, 2016-04-05 18:35:42]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[pavely, 2016-04-05 18:45:15]

The CQ bit was checked by pavely@chromium.org

[pavely, 2016-04-05 18:45:15]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org,
skym@chromium.org, maxbogue@chromium.org
Link to the patchset: https://codereview.chromium.org/1848793006/#ps80001
(title: "Fix build.")

[commit-bot: I haz the power, 2016-04-05 18:45:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848793006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848793006/80001

[commit-bot: I haz the power, 2016-04-05 19:38:15]

Description was changed from

==========
[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713
==========

to

==========
[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713
==========

[commit-bot: I haz the power, 2016-04-05 19:38:16]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-04-05 19:39:45]

Description was changed from

==========
[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713
==========

to

==========
[Sync] Register USS types with ModelTypeRegistry before download

In this change:
- Introduced RegisterWithBackend in DTC. It is called right after LoadModels
  but before first configure. It allows datatype to register with backend
  classes.
- Introduced GROUP_NON_BLOCKING model safe group. ModelTypeRegistry will not
  create directory based update handler and commit contributor for types in this
  group
- SyncBackendRegistrar keeps track of set of USS types. When configured, USS
  types are assigned to GROUP_NON_BLOCKING group. USS types should call
  SyncBackendRegistrar::RegisterNonBlockingType to be added to type set.

This change allows DeviceInfo to download initiali set of entries.
Initialization still fails because SyncManagerImpl can't discover that initial
sync is done for USS types, I'll address it in the next change.

R=zea@chromium.org
BUG=530713

Committed: https://crrev.com/7f2ee45b59370b511a22ba5f7be2c366c51e572a
Cr-Commit-Position: refs/heads/master@{#385257}
==========

[commit-bot: I haz the power, 2016-04-05 19:39:46]

Patchset 4 (id:??) landed as
https://crrev.com/7f2ee45b59370b511a22ba5f7be2c366c51e572a
Cr-Commit-Position: refs/heads/master@{#385257}