Introduce NonBlockingDataTypeController

Introduce NonBlockingDataTypeController

The NonBlockingDataTypeController tracks the sync state of
a non-blocking data type and sends signals to its components from the UI
thread.  It is responsible for enabling and disabling sync for a data
type according to user preference and the availbility of the sync
backend.

This CL also includes some changes to the NonBlockingDataTypeProcessor,
which is currently just a stub of what will eventually become
non-blocking sync's model thread component, so that it can be used in
testing NonBlockingDataTypeController.

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

[rlarocque, 2014-04-23 17:54:37]

Here's the next part of non-blocking sync.  Please review.

[tim (not reviewing), 2014-04-23 18:28:04]

It looks like (awesomely!) all your includes are from base/ and sync/, which
means it should be a straight copy-paste of your new files into
components/sync_driver, to avoid adding work for the componentization effort.

[Nicolas Zea, 2014-04-23 22:26:44]

Some initial comments

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.cc:36: proxy_ =
proxy->Clone();
Why clone? why not pass ownership?

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.cc:58: if
(GetDesiredState() == current_state_) {
This kind of logic concerns me a bit. It seems like the state variables are the
"truth". Rather, is_user_enabled, proxy_, processor_ denote the current "truth".
I wonder if you should just have a GetState() method that derives the state
based on those variables, and get rid of the current_state_ variable altogether.
Otherwise, if you modify one of those variables without updating state, things
could get confused.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:44: //
NonBlockingDataTypeController or SyncCoreProxy have not yet registered.
Presumably also during sync/datatype shutdown too, right? (i.e. if we for some
reason disable the temporarily datatype automatically) Perhaps mention that?

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:53: class
NonBlockingDataTypeController {
The description above is very useful

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:60: // This process
is irreversible.  It should happen at most once in the
Rather than describing this as irreversible, it might be more clear to state
that it's assumed the NonBlockingTypeProcessor will outlive the data type
controller (and sync itself). Therefore a datatype controller only ever deals
with a single type processor.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:72: void
ClearSyncCoreProxy();
perhaps DisconnectFromSync expresses the intent clearer? (and
InitializeSyncConnection rather than InitializeSyncCoreProxy) In general the
SyncCoreProxy is an implementation detail to me, so I'd prefer naming based on
the desired result.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:76: void
SetIsUserEnabled(bool is_user_enabled);
I'd like to not introduce a new notion of "user enabled" here. Should the data
type controller just listen to the sync prefs directly, and observe the
preferred state?

That said, what about types preferred but not enabled (e.g. encryption issues,
datatype errors, etc.)?

[rlarocque, 2014-04-23 23:28:34]

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.cc:36: proxy_ =
proxy->Clone();
On 2014/04/23 22:26:45, Nicolas Zea wrote:
> Why clone? why not pass ownership?

Passing ownership makes more sense.  Done.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.cc:58: if
(GetDesiredState() == current_state_) {
On 2014/04/23 22:26:45, Nicolas Zea wrote:
> This kind of logic concerns me a bit. It seems like the state variables are
the
> "truth". Rather, is_user_enabled, proxy_, processor_ denote the current
"truth".
> I wonder if you should just have a GetState() method that derives the state
> based on those variables, and get rid of the current_state_ variable
altogether.
> Otherwise, if you modify one of those variables without updating state, things
> could get confused.

The problem is that a lot of the complexity lives in state transitions, not the
states themselves.

The state variables represent a kind of "truth" as far as this class is
concerned, here on the UI thread.  As soon as InitializeSyncCoreProxy() is
called, this class can say IsSyncBackendConnected() == true.

But sync for this data type is not fully "connected" until the model thread and
sync thread are talking to each other.  This class needs to kick off that
connection process with a call to SendEnableSignal().  (Technically, just
calling that method won't actually result in sync for this type being
"connected", but it's close enough because we can safely make some assumptions
about the order in which tasks are run.)

In some cases, it doesn't make sense to send signals twice.  If something
happens and we transition from a DISCONNECTED state to a DISCONNECTED state,
there's no need to send signals twice.  In order to make the right decisions all
the time, we need to use one of:
- The current state and the previous state
- The current state and the knowledge of which event was just triggered

I experimented with both and found that the first of the two made more sense to
me.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:44: //
NonBlockingDataTypeController or SyncCoreProxy have not yet registered.
On 2014/04/23 22:26:45, Nicolas Zea wrote:
> Presumably also during sync/datatype shutdown too, right? (i.e. if we for some
> reason disable the temporarily datatype automatically) Perhaps mention that?

Whether or not we disconnect before shutdown is optional.  We should disconnect
if the sync backend goes down, though.  I'll add to the comment.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:60: // This process
is irreversible.  It should happen at most once in the
On 2014/04/23 22:26:45, Nicolas Zea wrote:
> Rather than describing this as irreversible, it might be more clear to state
> that it's assumed the NonBlockingTypeProcessor will outlive the data type
> controller (and sync itself). Therefore a datatype controller only ever deals
> with a single type processor.

I'm not sure that the NonBlockingTypeProcessor will outlive sync.  I think the
WeakPtrs make it safe for it to be shut down before the ProfileSyncService, if
it's fast enough.

The reason why there's no "undo" for this operation is that there's no way for a
NonBlockingTypeProcessor for a specific type to disappear then re-appear.

I rewrote part of this comment, but it's not much better than the original.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:72: void
ClearSyncCoreProxy();
On 2014/04/23 22:26:45, Nicolas Zea wrote:
> perhaps DisconnectFromSync expresses the intent clearer? (and
> InitializeSyncConnection rather than InitializeSyncCoreProxy) In general the
> SyncCoreProxy is an implementation detail to me, so I'd prefer naming based on
> the desired result.

I can see where you're going with that, but it's not entirely accurate.  The
name DisconnectFromSync would make a lot of sense for the processor.  The
controller was never really "connected" to sync to begin with.  It just keeps a
SyncCoreProxy around so that it can tell the processor about it when the
processor becomes available.  If we could guarantee that InitializeProcessor()
always precedes InitializeSyncCoreProxy(), then we wouldn't even need to keep a
SyncCoreProxy here.

In particular, if the processor had never connected to this controller, but we
call InitializeSyncCoreProxy() then ClearSyncCoreProxy(), the "clear" name is
accurate, since that's all the call does.  The name "disconnect" would not apply
in this case, since there was no connection to be torn down.

I'm open to other suggestions, but I don't think that DisconnectFromSync is the
right wording here.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:76: void
SetIsUserEnabled(bool is_user_enabled);
On 2014/04/23 22:26:45, Nicolas Zea wrote:
> I'd like to not introduce a new notion of "user enabled" here. Should the data
> type controller just listen to the sync prefs directly, and observe the
> preferred state?
> 
> That said, what about types preferred but not enabled (e.g. encryption issues,
> datatype errors, etc.)?

Good point.  "user enabled" is a bit too specific.

My thinking was that the "disabled" state would be reserved for cases is not
currently enabled, and not likely to be enabled in the future.  As part of the
transition to the disabled state, the data type is supposed to assume that the
server knows nothing about it, so it should clear all progress markers, version
numbers, etc.  I think encryption and data type errors don't quite qualify, but
some other cases might.

How about "IsPermanentlyDisabled"?

[rlarocque, 2014-04-24 21:18:55]

The latest upload seems to be doing well on the trybots.

ping/PTAL.

[Nicolas Zea, 2014-04-24 22:31:20]

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.cc (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.cc:58: if
(GetDesiredState() == current_state_) {
On 2014/04/23 23:28:35, rlarocque wrote:
> On 2014/04/23 22:26:45, Nicolas Zea wrote:
> > This kind of logic concerns me a bit. It seems like the state variables are
> the
> > "truth". Rather, is_user_enabled, proxy_, processor_ denote the current
> "truth".
> > I wonder if you should just have a GetState() method that derives the state
> > based on those variables, and get rid of the current_state_ variable
> altogether.
> > Otherwise, if you modify one of those variables without updating state,
things
> > could get confused.
> 
> The problem is that a lot of the complexity lives in state transitions, not
the
> states themselves.
> 
> The state variables represent a kind of "truth" as far as this class is
> concerned, here on the UI thread.  As soon as InitializeSyncCoreProxy() is
> called, this class can say IsSyncBackendConnected() == true.
> 
> But sync for this data type is not fully "connected" until the model thread
and
> sync thread are talking to each other.  This class needs to kick off that
> connection process with a call to SendEnableSignal().  (Technically, just
> calling that method won't actually result in sync for this type being
> "connected", but it's close enough because we can safely make some assumptions
> about the order in which tasks are run.)
> 
> In some cases, it doesn't make sense to send signals twice.  If something
> happens and we transition from a DISCONNECTED state to a DISCONNECTED state,
> there's no need to send signals twice.  In order to make the right decisions
all
> the time, we need to use one of:
> - The current state and the previous state
> - The current state and the knowledge of which event was just triggered
> 
> I experimented with both and found that the first of the two made more sense
to
> me.

I see. Now that I read your comment on current_state_ more carefully, this is
clearer. SGTM

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:72: void
ClearSyncCoreProxy();
On 2014/04/23 23:28:35, rlarocque wrote:
> On 2014/04/23 22:26:45, Nicolas Zea wrote:
> > perhaps DisconnectFromSync expresses the intent clearer? (and
> > InitializeSyncConnection rather than InitializeSyncCoreProxy) In general the
> > SyncCoreProxy is an implementation detail to me, so I'd prefer naming based
on
> > the desired result.
> 
> I can see where you're going with that, but it's not entirely accurate.  The
> name DisconnectFromSync would make a lot of sense for the processor.  The
> controller was never really "connected" to sync to begin with.  It just keeps
a
> SyncCoreProxy around so that it can tell the processor about it when the
> processor becomes available.  If we could guarantee that InitializeProcessor()
> always precedes InitializeSyncCoreProxy(), then we wouldn't even need to keep
a
> SyncCoreProxy here.
> 
> In particular, if the processor had never connected to this controller, but we
> call InitializeSyncCoreProxy() then ClearSyncCoreProxy(), the "clear" name is
> accurate, since that's all the call does.  The name "disconnect" would not
apply
> in this case, since there was no connection to be torn down.
> 
> I'm open to other suggestions, but I don't think that DisconnectFromSync is
the
> right wording here.

I see. Okay, in that case, given that the controller is really just acting as an
escrow in some sense, this makes more sense.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:76: void
SetIsUserEnabled(bool is_user_enabled);
On 2014/04/23 23:28:35, rlarocque wrote:
> On 2014/04/23 22:26:45, Nicolas Zea wrote:
> > I'd like to not introduce a new notion of "user enabled" here. Should the
data
> > type controller just listen to the sync prefs directly, and observe the
> > preferred state?
> > 
> > That said, what about types preferred but not enabled (e.g. encryption
issues,
> > datatype errors, etc.)?
> 
> Good point.  "user enabled" is a bit too specific.
> 
> My thinking was that the "disabled" state would be reserved for cases is not
> currently enabled, and not likely to be enabled in the future.  As part of the
> transition to the disabled state, the data type is supposed to assume that the
> server knows nothing about it, so it should clear all progress markers,
version
> numbers, etc.  I think encryption and data type errors don't quite qualify,
but
> some other cases might.
> 
> How about "IsPermanentlyDisabled"?

Isn't that the same as not preferred? I see preferred as denoting whether the
type "should be" enabled at some point. Enabled/Disabled are more transient
states, and don't necessarily correlate to whether the type has sync data or
not. Preferred/Not preferred are what decide whether we should throw away the
sync data or not.

I'd mainly just like to make sure the terminology, and what controls the state
is clear. We have lots of different states a particular type can be in (we want
it enabled but it's waiting on the type, we want it enabled but had to disable
because of error, we don't want it enabled right now but we do once we finish
enabling other types, etc.)

https://codereview.chromium.org/249843002/diff/80001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/80001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:80: bool
ShouldEnableInvalidations() const;
This method doesn't appear to exist in the cc file?

https://codereview.chromium.org/249843002/diff/80001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:129:
scoped_ptr<syncer::SyncCoreProxy> proxy_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/249843002/diff/80001/sync/internal_api/non_bl...
File sync/internal_api/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/249843002/diff/80001/sync/internal_api/non_bl...
sync/internal_api/non_blocking_type_processor.cc:40:
core_proxy->ConnectTypeToCore(
nit: can this be combined on one line?

[rlarocque, 2014-04-25 17:44:47]

Patch updated.  PTAL.

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/40001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:76: void
SetIsUserEnabled(bool is_user_enabled);
On 2014/04/24 22:31:21, Nicolas Zea wrote:
> On 2014/04/23 23:28:35, rlarocque wrote:
> > On 2014/04/23 22:26:45, Nicolas Zea wrote:
> > > I'd like to not introduce a new notion of "user enabled" here. Should the
> data
> > > type controller just listen to the sync prefs directly, and observe the
> > > preferred state?
> > > 
> > > That said, what about types preferred but not enabled (e.g. encryption
> issues,
> > > datatype errors, etc.)?
> > 
> > Good point.  "user enabled" is a bit too specific.
> > 
> > My thinking was that the "disabled" state would be reserved for cases is not
> > currently enabled, and not likely to be enabled in the future.  As part of
the
> > transition to the disabled state, the data type is supposed to assume that
the
> > server knows nothing about it, so it should clear all progress markers,
> version
> > numbers, etc.  I think encryption and data type errors don't quite qualify,
> but
> > some other cases might.
> > 
> > How about "IsPermanentlyDisabled"?
> 
> Isn't that the same as not preferred? I see preferred as denoting whether the
> type "should be" enabled at some point. Enabled/Disabled are more transient
> states, and don't necessarily correlate to whether the type has sync data or
> not. Preferred/Not preferred are what decide whether we should throw away the
> sync data or not.
> 
> I'd mainly just like to make sure the terminology, and what controls the state
> is clear. We have lots of different states a particular type can be in (we
want
> it enabled but it's waiting on the type, we want it enabled but had to disable
> because of error, we don't want it enabled right now but we do once we finish
> enabling other types, etc.)

It sounds like the disagreement is entirely due to terminology.  I'll switch to
the term "preferred" here and in the NonBlockingDataTypeProcessor.

https://codereview.chromium.org/249843002/diff/80001/components/sync_driver/n...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/80001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:80: bool
ShouldEnableInvalidations() const;
On 2014/04/24 22:31:21, Nicolas Zea wrote:
> This method doesn't appear to exist in the cc file?

Good point.  That feature has been deferred until a future CL.  Removed.

https://codereview.chromium.org/249843002/diff/80001/components/sync_driver/n...
components/sync_driver/non_blocking_data_type_controller.h:129:
scoped_ptr<syncer::SyncCoreProxy> proxy_;
On 2014/04/24 22:31:21, Nicolas Zea wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/249843002/diff/80001/sync/internal_api/non_bl...
File sync/internal_api/non_blocking_type_processor.cc (right):

https://codereview.chromium.org/249843002/diff/80001/sync/internal_api/non_bl...
sync/internal_api/non_blocking_type_processor.cc:40:
core_proxy->ConnectTypeToCore(
On 2014/04/24 22:31:21, Nicolas Zea wrote:
> nit: can this be combined on one line?

Yep.  Done.

[Nicolas Zea, 2014-04-25 20:47:28]

LGTM

https://codereview.chromium.org/249843002/diff/100001/components/sync_driver/...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/100001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller.h:56:
NonBlockingDataTypeController(syncer::ModelType type, bool is_preferred);
nit: style guide prefers using an enum rather than a bool for this kind of
parameter.

https://codereview.chromium.org/249843002/diff/100001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller.h:97: // Retruns true
if this type should be synced.
nit: returns

[rlarocque, 2014-04-25 21:03:46]

https://codereview.chromium.org/249843002/diff/100001/components/sync_driver/...
File components/sync_driver/non_blocking_data_type_controller.h (right):

https://codereview.chromium.org/249843002/diff/100001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller.h:56:
NonBlockingDataTypeController(syncer::ModelType type, bool is_preferred);
On 2014/04/25 20:47:28, Nicolas Zea wrote:
> nit: style guide prefers using an enum rather than a bool for this kind of
> parameter.

I don't think a bool would be a good fit here.  We'd need to add code to convert
the pref value to an enum, then convert it back to a bool for use in this class
(or handle the conversion in SetIsPreferred...)

I think it's better to leave it as a bool for now.  We can revisit in the next
CL, where the class is actually instantiated, if necessary.

https://codereview.chromium.org/249843002/diff/100001/components/sync_driver/...
components/sync_driver/non_blocking_data_type_controller.h:97: // Retruns true
if this type should be synced.
On 2014/04/25 20:47:28, Nicolas Zea wrote:
> nit: returns

Done.

[rlarocque, 2014-04-25 21:04:01]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:02:24]

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

[commit-bot: I haz the power, 2014-04-26 09:57:49]

Change committed as 266344