[Sync] Add sync api support for context changes

[Sync] Add sync api support for context changes

The sync engine/api won't act on them yet; this just lays the groundwork for
passing context changes around.

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

[Nicolas Zea, 2014-03-31 21:31:25]

Nick, PTAL

[maniscalco, 2014-03-31 23:59:15]

Just FYI, I have two CLs about to land that touch SyncData:

https://codereview.chromium.org/187303006/

and

https://codereview.chromium.org/217063005/

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.cc
File sync/api/sync_change.cc (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.cc#...
sync/api/sync_change.cc:76: default:
Needs to be updated?

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h
File sync/api/sync_change.h (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:19: // change is to a a sync entity, the change can be
either a delete, add, or an
nit: s/to a a/to a/

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:19: // change is to a a sync entity, the change can be
either a delete, add, or an
nit: s/either //

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:31: CONTEXT_UPDATE,
It would be helpful to have a comment somewhere (here or elsewhere) that
explains what a context update is.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:31: CONTEXT_UPDATE,
I would have expected this to be called ACTION_CONTEXT_UPDATE.  Is there a
reason to avoid calling it that?  I guess I'm still fuzzy on how to think about
a context update relative to other SyncChanges.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.cc
File sync/api/sync_data.cc (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.cc#ne...
sync/api/sync_data.cc:207: "specifics: " + specifics + "}";
Need a comma and space before specifics?

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype, const
std::string& context);
Any reason not to pass datatype by const ref?  The one advantage I see to
passing via const ref is that if ModelType ever becomes a big class, we don't
have to worry about unnecessary temporaries.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype, const
std::string& context);
Are SyncData created by this method "local" or "remote"?  Or neither?

[Nicolas Zea, 2014-04-02 18:34:36]

Comments addressed, PTAL.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.cc
File sync/api/sync_change.cc (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.cc#...
sync/api/sync_change.cc:76: default:
On 2014/03/31 23:59:16, maniscalco wrote:
> Needs to be updated?

Woops, good catch, done.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h
File sync/api/sync_change.h (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:19: // change is to a a sync entity, the change can be
either a delete, add, or an
On 2014/03/31 23:59:16, maniscalco wrote:
> nit: s/to a a/to a/

Done.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:19: // change is to a a sync entity, the change can be
either a delete, add, or an
On 2014/03/31 23:59:16, maniscalco wrote:
> nit: s/either //

Done.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:31: CONTEXT_UPDATE,
On 2014/03/31 23:59:16, maniscalco wrote:
> I would have expected this to be called ACTION_CONTEXT_UPDATE.  Is there a
> reason to avoid calling it that?  I guess I'm still fuzzy on how to think
about
> a context update relative to other SyncChanges.

I was kind of thinking to leave the ACTION_ prefixes for those changes to actual
sync entities. WDYT?

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:31: CONTEXT_UPDATE,
On 2014/03/31 23:59:16, maniscalco wrote:
> It would be helpful to have a comment somewhere (here or elsewhere) that
> explains what a context update is.

Done.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.cc
File sync/api/sync_data.cc (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.cc#ne...
sync/api/sync_data.cc:207: "specifics: " + specifics + "}";
On 2014/03/31 23:59:16, maniscalco wrote:
> Need a comma and space before specifics?

Done.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype, const
std::string& context);
On 2014/03/31 23:59:16, maniscalco wrote:
> Any reason not to pass datatype by const ref?  The one advantage I see to
> passing via const ref is that if ModelType ever becomes a big class, we don't
> have to worry about unnecessary temporaries.

ModelType is just an enum, so passing by ref is actually more expensive (passing
full pointer vs small value). And I don't think there are any plans to change
the ModelType's class in the near future, so we typically prefer to pass by
value.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype, const
std::string& context);
On 2014/03/31 23:59:16, maniscalco wrote:
> Are SyncData created by this method "local" or "remote"?  Or neither?

Neither. I was actually planning to get rid of the whole IsLocal() method, which
is only used in one place (generic_change_processor).

[maniscalco, 2014-04-02 21:13:09]

I'm thinking through the implications of modeling datatype context update as a
new type of SyncChange.  In some ways it feels like a good fit, but in others it
seems a bit awkward.  Two things come to mind.

1. Anywhere we branch on SyncChangeType we'll need to look at the code and
determine how it should behave in the face of CONTEXT_UPDATE SyncDatas.  E.g.
GenericChangeProcessor::ApplyChangesFromSyncModel has an else clauses where it
assumes a SyncData must be an ACTION_UPDATE (by process of elimination).  I
think there may be more cases like this elsewhere.

2. This change will remove some invariants of SyncData.  E.g. prior to this
patch all SyncData are either "local" or "remote".  All "local" SyncData (those
going *to* the syncer) have a sync_tag (SyncData::GetTag).  After this patch,
that won't always be the case because asking for the tag of a SyncData that
represents a context update doesn't make sense.  I'm not sure what GetTag should
do in that case.  CHECK/DCHECK?

I want to run another implementation option by you.  What do you think about
implementing context updates using the SyncChangeProcessor interface?  SCP would
get a new method:

SyncChangeProcessor::UpdateContext(const std::string& context);

When datatype code wants to update its context, it would call UpdateContext on
the SCP it received in MergeDataAndStartSyncing.

The thing I like about this approach is that we don't need to weaken the
invariants of SyncData and we don't have to worry about making sure existing
code properly handles CONTEXT_UPDATE SyncChanges.  One problem I see here is
that SyncableService implements the SyncChangeProcessor interface and it
probably doesn't make sense to call UpdateContext on a SyncableService.  Seems
like if we go this route, we may need to break the symmetry of
SyncChangeProcessor (it's used in both directions, datatype -> syncer and syncer
-> datatype).

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h
File sync/api/sync_change.h (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
sync/api/sync_change.h:31: CONTEXT_UPDATE,
On 2014/04/02 18:34:36, Nicolas Zea wrote:
> On 2014/03/31 23:59:16, maniscalco wrote:
> > I would have expected this to be called ACTION_CONTEXT_UPDATE.  Is there a
> > reason to avoid calling it that?  I guess I'm still fuzzy on how to think
> about
> > a context update relative to other SyncChanges.
> 
> I was kind of thinking to leave the ACTION_ prefixes for those changes to
actual
> sync entities. WDYT?

I'm OK with that distinction.  My thinking here was that it's handy to have a
way to easily search for all uses of the SyncChangeType enum (e.g. grep
SyncChange::ACTION_).  I guess having CONTEXT_UPDATE doesn't change the
searchability all that much because we can search for SyncChange::ACTION_ and
SyncChange::CONTEXT_.

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h
File sync/api/sync_data.h (right):

https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h#new...
sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype, const
std::string& context);
On 2014/04/02 18:34:36, Nicolas Zea wrote:
> On 2014/03/31 23:59:16, maniscalco wrote:
> > Any reason not to pass datatype by const ref?  The one advantage I see to
> > passing via const ref is that if ModelType ever becomes a big class, we
don't
> > have to worry about unnecessary temporaries.
> 
> ModelType is just an enum, so passing by ref is actually more expensive
(passing
> full pointer vs small value). And I don't think there are any plans to change
> the ModelType's class in the near future, so we typically prefer to pass by
> value.

Pass by value SGTM.

[Nicolas Zea, 2014-04-02 22:16:46]

On 2014/04/02 21:13:09, maniscalco wrote:
> I'm thinking through the implications of modeling datatype context update as a
> new type of SyncChange.  In some ways it feels like a good fit, but in others
it
> seems a bit awkward.  Two things come to mind.
> 
> 1. Anywhere we branch on SyncChangeType we'll need to look at the code and
> determine how it should behave in the face of CONTEXT_UPDATE SyncDatas.  E.g.
> GenericChangeProcessor::ApplyChangesFromSyncModel has an else clauses where it
> assumes a SyncData must be an ACTION_UPDATE (by process of elimination).  I
> think there may be more cases like this elsewhere.
> 
> 2. This change will remove some invariants of SyncData.  E.g. prior to this
> patch all SyncData are either "local" or "remote".  All "local" SyncData
(those
> going *to* the syncer) have a sync_tag (SyncData::GetTag).  After this patch,
> that won't always be the case because asking for the tag of a SyncData that
> represents a context update doesn't make sense.  I'm not sure what GetTag
should
> do in that case.  CHECK/DCHECK?
> 
> I want to run another implementation option by you.  What do you think about
> implementing context updates using the SyncChangeProcessor interface?  SCP
would
> get a new method:
> 
> SyncChangeProcessor::UpdateContext(const std::string& context);
> 
> When datatype code wants to update its context, it would call UpdateContext on
> the SCP it received in MergeDataAndStartSyncing.
> 
> The thing I like about this approach is that we don't need to weaken the
> invariants of SyncData and we don't have to worry about making sure existing
> code properly handles CONTEXT_UPDATE SyncChanges.  One problem I see here is
> that SyncableService implements the SyncChangeProcessor interface and it
> probably doesn't make sense to call UpdateContext on a SyncableService.  Seems
> like if we go this route, we may need to break the symmetry of
> SyncChangeProcessor (it's used in both directions, datatype -> syncer and
syncer
> -> datatype).
> 

I like this idea. And, in fact, UpdateContext can go both ways, since the server
can change the context. For now though, I can just have an empty default
implementation. Will update patch.

> https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h
> File sync/api/sync_change.h (right):
> 
>
https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_change.h#n...
> sync/api/sync_change.h:31: CONTEXT_UPDATE,
> On 2014/04/02 18:34:36, Nicolas Zea wrote:
> > On 2014/03/31 23:59:16, maniscalco wrote:
> > > I would have expected this to be called ACTION_CONTEXT_UPDATE.  Is there a
> > > reason to avoid calling it that?  I guess I'm still fuzzy on how to think
> > about
> > > a context update relative to other SyncChanges.
> > 
> > I was kind of thinking to leave the ACTION_ prefixes for those changes to
> actual
> > sync entities. WDYT?
> 
> I'm OK with that distinction.  My thinking here was that it's handy to have a
> way to easily search for all uses of the SyncChangeType enum (e.g. grep
> SyncChange::ACTION_).  I guess having CONTEXT_UPDATE doesn't change the
> searchability all that much because we can search for SyncChange::ACTION_ and
> SyncChange::CONTEXT_.
> 
> https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h
> File sync/api/sync_data.h (right):
> 
>
https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h#new...
> sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype,
const
> std::string& context);
> On 2014/04/02 18:34:36, Nicolas Zea wrote:
> > On 2014/03/31 23:59:16, maniscalco wrote:
> > > Any reason not to pass datatype by const ref?  The one advantage I see to
> > > passing via const ref is that if ModelType ever becomes a big class, we
> don't
> > > have to worry about unnecessary temporaries.
> > 
> > ModelType is just an enum, so passing by ref is actually more expensive
> (passing
> > full pointer vs small value). And I don't think there are any plans to
change
> > the ModelType's class in the near future, so we typically prefer to pass by
> > value.
> 
> Pass by value SGTM.

[maniscalco, 2014-04-02 22:34:27]

Great!  Ah, right, UpdateContext does make sense in both directions.


On Wed, Apr 2, 2014 at 3:16 PM, <zea@chromium.org> wrote:

> On 2014/04/02 21:13:09, maniscalco wrote:
>
>> I'm thinking through the implications of modeling datatype context update
>> as a
>> new type of SyncChange.  In some ways it feels like a good fit, but in
>> others
>>
> it
>
>> seems a bit awkward.  Two things come to mind.
>>
>
>  1. Anywhere we branch on SyncChangeType we'll need to look at the code and
>> determine how it should behave in the face of CONTEXT_UPDATE SyncDatas.
>>  E.g.
>> GenericChangeProcessor::ApplyChangesFromSyncModel has an else clauses
>> where it
>> assumes a SyncData must be an ACTION_UPDATE (by process of elimination).
>>  I
>> think there may be more cases like this elsewhere.
>>
>
>  2. This change will remove some invariants of SyncData.  E.g. prior to
>> this
>> patch all SyncData are either "local" or "remote".  All "local" SyncData
>>
> (those
>
>> going *to* the syncer) have a sync_tag (SyncData::GetTag).  After this
>> patch,
>> that won't always be the case because asking for the tag of a SyncData
>> that
>> represents a context update doesn't make sense.  I'm not sure what GetTag
>>
> should
>
>> do in that case.  CHECK/DCHECK?
>>
>
>  I want to run another implementation option by you.  What do you think
>> about
>> implementing context updates using the SyncChangeProcessor interface?  SCP
>>
> would
>
>> get a new method:
>>
>
>  SyncChangeProcessor::UpdateContext(const std::string& context);
>>
>
>  When datatype code wants to update its context, it would call
>> UpdateContext on
>> the SCP it received in MergeDataAndStartSyncing.
>>
>
>  The thing I like about this approach is that we don't need to weaken the
>> invariants of SyncData and we don't have to worry about making sure
>> existing
>> code properly handles CONTEXT_UPDATE SyncChanges.  One problem I see here
>> is
>> that SyncableService implements the SyncChangeProcessor interface and it
>> probably doesn't make sense to call UpdateContext on a SyncableService.
>>  Seems
>> like if we go this route, we may need to break the symmetry of
>> SyncChangeProcessor (it's used in both directions, datatype -> syncer and
>>
> syncer
>
>> -> datatype).
>>
>
>
> I like this idea. And, in fact, UpdateContext can go both ways, since the
> server
> can change the context. For now though, I can just have an empty default
> implementation. Will update patch.
>
>
>  https://codereview.chromium.org/220043002/diff/20001/sync/
>> api/sync_change.h
>> File sync/api/sync_change.h (right):
>>
>
>
> https://codereview.chromium.org/220043002/diff/20001/sync/
> api/sync_change.h#newcode31
>
>> sync/api/sync_change.h:31: CONTEXT_UPDATE,
>> On 2014/04/02 18:34:36, Nicolas Zea wrote:
>> > On 2014/03/31 23:59:16, maniscalco wrote:
>> > > I would have expected this to be called ACTION_CONTEXT_UPDATE.  Is
>> there a
>> > > reason to avoid calling it that?  I guess I'm still fuzzy on how to
>> think
>> > about
>> > > a context update relative to other SyncChanges.
>> >
>> > I was kind of thinking to leave the ACTION_ prefixes for those changes
>> to
>> actual
>> > sync entities. WDYT?
>>
>
>  I'm OK with that distinction.  My thinking here was that it's handy to
>> have a
>> way to easily search for all uses of the SyncChangeType enum (e.g. grep
>> SyncChange::ACTION_).  I guess having CONTEXT_UPDATE doesn't change the
>> searchability all that much because we can search for SyncChange::ACTION_
>> and
>> SyncChange::CONTEXT_.
>>
>
>  https://codereview.chromium.org/220043002/diff/20001/sync/api/sync_data.h
>> File sync/api/sync_data.h (right):
>>
>
>
> https://codereview.chromium.org/220043002/diff/20001/sync/
> api/sync_data.h#newcode61
>
>> sync/api/sync_data.h:61: static SyncData CreateContext(ModelType datatype,
>>
> const
>
>> std::string& context);
>> On 2014/04/02 18:34:36, Nicolas Zea wrote:
>> > On 2014/03/31 23:59:16, maniscalco wrote:
>> > > Any reason not to pass datatype by const ref?  The one advantage I
>> see to
>> > > passing via const ref is that if ModelType ever becomes a big class,
>> we
>> don't
>> > > have to worry about unnecessary temporaries.
>> >
>> > ModelType is just an enum, so passing by ref is actually more expensive
>> (passing
>> > full pointer vs small value). And I don't think there are any plans to
>>
> change
>
>> > the ModelType's class in the near future, so we typically prefer to
>> pass by
>> > value.
>>
>
>  Pass by value SGTM.
>>
>
>
>
> https://codereview.chromium.org/220043002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nicolas Zea, 2014-04-02 22:36:55]

PTAL

[maniscalco, 2014-04-02 22:50:07]

On 2014/04/02 22:36:55, Nicolas Zea wrote:
> PTAL

LGTM.  BTW, the comment you added in sync_change_processor.h is helpful. 
Likening context to a cookie makes it easier to understand its purpose.

[Nicolas Zea, 2014-04-02 23:23:50]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:29:50]

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

[commit-bot: I haz the power, 2014-04-02 23:29:52]

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

[commit-bot: I haz the power, 2014-04-02 23:29:52]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Nicolas Zea, 2014-04-02 23:34:04]

+Richard for sync stamp

[rlarocque, 2014-04-02 23:44:45]

lgtm

[Nicolas Zea, 2014-04-02 23:50:28]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:52:35]

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

[commit-bot: I haz the power, 2014-04-03 08:32:50]

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

[commit-bot: I haz the power, 2014-04-03 08:32:52]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Nicolas Zea, 2014-04-03 17:45:27]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-04-03 17:46:06]

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

[commit-bot: I haz the power, 2014-04-03 17:47:57]

Change committed as 261469