[Sync] Add support for dynamically enabling/disabling types

[Sync] Add support for dynamically enabling/disabling types

The notion of Unready types is also introduced for those types which control whether
they should run, and therefore for which a configuration (which might
involve network access) should not even be performed.

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

[Nicolas Zea, 2014-06-04 21:20:50]

PTAL

[Nicolas Zea, 2014-06-05 21:02:29]

ping?

[haitaol1, 2014-06-05 22:24:21]

https://codereview.chromium.org/312163004/diff/20001/components/sync_driver/d...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/312163004/diff/20001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:150: const
syncer::ModelTypeSet& types_being_configured) const {
Should types_being_configured not contain unready types because they are not
added to filtered_desired_types above?

[stanisc, 2014-06-10 21:21:42]

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

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:101:
failed_data_types_handler_->UpdateFailedDataTypes(errors);
Consider adding an overload for UpdateFailedDataTypes that takes just one error.
That seems to be common enough.

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
File components/sync_driver/failed_data_types_handler.cc (right):

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
components/sync_driver/failed_data_types_handler.cc:42: NOTREACHED();
Wouldn't it be better to keep the default case instead of the specific UNSET
case?

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
File components/sync_driver/failed_data_types_handler.h (right):

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
components/sync_driver/failed_data_types_handler.h:85: // be marked as ready by
calling ResetDataTypeErrorFor(..).
ResetDataTypeErrorFor or ResetUnreadyErrorFor?

https://codereview.chromium.org/312163004/diff/40001/sync/api/sync_error.h
File sync/api/sync_error.h (right):

https://codereview.chromium.org/312163004/diff/40001/sync/api/sync_error.h#ne...
sync/api/sync_error.h:41: UNREADY_ERROR,        // The type is not ready to
start yet, so should be
I didn't quite understand whether this could be used later to explicitly disable
a datatype due to configuration constraints or user settings. If that is the
plan, should this error type be called differently, for instance DISABLED_ERROR?
UNREADY sounds like it could become ready at some point which probably isn't the
case for types that are disabled by constraints.

[Nicolas Zea, 2014-06-19 00:04:23]

Comments addressed, PTAL

https://codereview.chromium.org/312163004/diff/20001/components/sync_driver/d...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/312163004/diff/20001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:150: const
syncer::ModelTypeSet& types_being_configured) const {
On 2014/06/05 22:24:21, haitaol1 wrote:
> Should types_being_configured not contain unready types because they are not
> added to filtered_desired_types above?

You're right, fixed in the most recent patch (and added some logic to ensure we
don't purge the unready types).

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

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:101:
failed_data_types_handler_->UpdateFailedDataTypes(errors);
On 2014/06/10 21:21:42, stanisc wrote:
> Consider adding an overload for UpdateFailedDataTypes that takes just one
error.
> That seems to be common enough.

I'd like to avoid that for this patch. I plan on refactoring the failed data
types handler significantly later, which will also involve cleaning up some of
this code.

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
File components/sync_driver/failed_data_types_handler.cc (right):

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
components/sync_driver/failed_data_types_handler.cc:42: NOTREACHED();
On 2014/06/10 21:21:42, stanisc wrote:
> Wouldn't it be better to keep the default case instead of the specific UNSET
> case?

I prefer to not have default cases, as you'll then get a compile error if you
forget to handle a case (rather than a runtime error).

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
File components/sync_driver/failed_data_types_handler.h (right):

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
components/sync_driver/failed_data_types_handler.h:85: // be marked as ready by
calling ResetDataTypeErrorFor(..).
On 2014/06/10 21:21:42, stanisc wrote:
> ResetDataTypeErrorFor or ResetUnreadyErrorFor?

Done.

https://codereview.chromium.org/312163004/diff/40001/sync/api/sync_error.h
File sync/api/sync_error.h (right):

https://codereview.chromium.org/312163004/diff/40001/sync/api/sync_error.h#ne...
sync/api/sync_error.h:41: UNREADY_ERROR,        // The type is not ready to
start yet, so should be
On 2014/06/10 21:21:42, stanisc wrote:
> I didn't quite understand whether this could be used later to explicitly
disable
> a datatype due to configuration constraints or user settings. If that is the
> plan, should this error type be called differently, for instance
DISABLED_ERROR?
> UNREADY sounds like it could become ready at some point which probably isn't
the
> case for types that are disabled by constraints.

I'm open to other names, but note that the type being disabled is different than
this error. If the type is disabled, we'll actively purge it everywhere. Unready
errors imply that some run time constraint has not been met, but once it does we
will be syncing the type again. In that case, don't purge it at this time.

In other words, when you go into user settings and explicitly disable the type,
a different code path will be used that the one involving this error.

[stanisc, 2014-06-19 17:07:03]

LGTM

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
File components/sync_driver/failed_data_types_handler.cc (right):

https://codereview.chromium.org/312163004/diff/40001/components/sync_driver/f...
components/sync_driver/failed_data_types_handler.cc:42: NOTREACHED();
On 2014/06/19 00:04:22, Nicolas Zea wrote:
> On 2014/06/10 21:21:42, stanisc wrote:
> > Wouldn't it be better to keep the default case instead of the specific UNSET
> > case?
> 
> I prefer to not have default cases, as you'll then get a compile error if you
> forget to handle a case (rather than a runtime error).
OK. I haven't realized the compiler would error out in that case because that
isn't default behavior.

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.h:64: struct Event;
I didn't see Event used below? Do you really need this?

[haitaol1, 2014-06-19 18:14:35]

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:354:
disabled_types.PutAll(unready_types);
This will remove routing info of unready types in registar. If unready type is
disabled later, it seems the the type won't be added to types_to_purge below
because types_to_purge is calculated by differing previous and current types in
routing info. Actually I wonder if there's a bug with crypto_types because of
this.

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

https://codereview.chromium.org/312163004/diff/100001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:209:
BackendDataTypeConfigurer::UNREADY, unready_types,
This is different form the comment #6 above.

[Nicolas Zea, 2014-06-19 21:13:02]

PTAL

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/glu...
File chrome/browser/sync/glue/sync_backend_host_impl.cc (right):

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/glu...
chrome/browser/sync/glue/sync_backend_host_impl.cc:354:
disabled_types.PutAll(unready_types);
On 2014/06/19 18:14:35, haitaol1 wrote:
> This will remove routing info of unready types in registar. If unready type is
> disabled later, it seems the the type won't be added to types_to_purge below
> because types_to_purge is calculated by differing previous and current types
in
> routing info. Actually I wonder if there's a bug with crypto_types because of
> this.

Hmm, I suspect you're right. They'll both be purged on the next restart though,
as part of the first configuration, so it's not too problematic. That said, I'll
file a bug to fix that separately, since it's an existing issue and would
probably require some further logic.

Added todo.

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/312163004/diff/100001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service.h:64: struct Event;
On 2014/06/19 17:07:03, stanisc wrote:
> I didn't see Event used below? Do you really need this?

Wasn't added by this patch, but removed.

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

https://codereview.chromium.org/312163004/diff/100001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:209:
BackendDataTypeConfigurer::UNREADY, unready_types,
On 2014/06/19 18:14:35, haitaol1 wrote:
> This is different form the comment #6 above.

Done.

[haitaol1, 2014-06-19 22:25:26]

lgtm

[Nicolas Zea, 2014-06-19 22:26:28]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-06-19 22:30:56]

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

[commit-bot: I haz the power, 2014-06-20 09:48:57]

Change committed as 278658