sync: Enable typed tombstones commits

sync: Enable typed tombstones commits

Modifies commit logic to include the model type of items in commit
requests.  The model type is implicitly specified by the presence of
a sub-message in the EntitySpecifics field.

Prior to this commit, only bookmarks would populate the sync entities on
commit.  The special case for bookmarks was introduced recently, in
r265587.

Unlike the bookmarks case, the non-bookmark items send up only enough
specifics to identify the model type.  The rest of the specifics are not
set.

This commit also adds some tests of the bookmark and non-bookmark
deletion request behaviors.

BUG=365752, 373859
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271763

[rlarocque, 2014-05-19 20:26:29]

Please review.

[haitaol1, 2014-05-19 21:59:11]

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc#n...
sync/engine/commit_util.cc:187: AddDefaultFieldValue(meta_entry.GetModelType(),
Will it cause parse failure on server if required field in data type specifics
is not set?

https://codereview.chromium.org/298503002/diff/1/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/298503002/diff/1/sync/protocol/sync.proto#new...
sync/protocol/sync.proto:695: optional int32 protocol_version = 2 [default =
33];
Do you have to change this? How is this used on server?

[albertb, 2014-05-19 22:02:57]

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc#n...
sync/engine/commit_util.cc:187: AddDefaultFieldValue(meta_entry.GetModelType(),
On 2014/05/19 21:59:12, haitaol1 wrote:
> Will it cause parse failure on server if required field in data type specifics
> is not set?

It would, but as far as I can tell, there are no required fields in any of the
specifics messages.

[rlarocque, 2014-05-19 22:14:57]

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc#n...
sync/engine/commit_util.cc:187: AddDefaultFieldValue(meta_entry.GetModelType(),
On 2014/05/19 21:59:12, haitaol1 wrote:
> Will it cause parse failure on server if required field in data type specifics
> is not set?

That's a good point.  It probably would cause a parse failure.  It might even
cause an error earlier, when we try to serialize the incomplete proto.

Fortunately, there are no required fields in specifics.

If there were required fields, I think it would be AddDefaultFieldValue's
responsibility to populate them with sensible defaults.  Otherwise, it would
likely cause problems elsewhere, too.

https://codereview.chromium.org/298503002/diff/1/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

https://codereview.chromium.org/298503002/diff/1/sync/protocol/sync.proto#new...
sync/protocol/sync.proto:695: optional int32 protocol_version = 2 [default =
33];
On 2014/05/19 21:59:12, haitaol1 wrote:
> Do you have to change this? How is this used on server?

This is used to tell the server when semantics for certain fields have changed. 
This way it can throw an error if (protocol_version >= 33 && GetModelType() ==
UNSPECIFIED).

Technically, this is bad protobuf hygiene.  Protobufs are not supposed to rely
on version numbers.  There is a "right" solution where we would not need to use
version numbers.  It would be a lot of work to implement, and it would waste a
few bytes.  I doubt I could convince anyone to implement it that way.

[haitaol1, 2014-05-19 22:39:05]

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc#n...
sync/engine/commit_util.cc:187: AddDefaultFieldValue(meta_entry.GetModelType(),
Can you add a test that calls AddDefaultFieldValue on all data types and
verifies for the modified specifics:
1. No parse error.
2. Correct model type can be deducted.

On 2014/05/19 22:14:57, rlarocque wrote:
> On 2014/05/19 21:59:12, haitaol1 wrote:
> > Will it cause parse failure on server if required field in data type
specifics
> > is not set?
> 
> That's a good point.  It probably would cause a parse failure.  It might even
> cause an error earlier, when we try to serialize the incomplete proto.
> 
> Fortunately, there are no required fields in specifics.
> 
> If there were required fields, I think it would be AddDefaultFieldValue's
> responsibility to populate them with sensible defaults.  Otherwise, it would
> likely cause problems elsewhere, too.

[rlarocque, 2014-05-19 23:51:28]

PTAL.

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc
File sync/engine/commit_util.cc (right):

https://codereview.chromium.org/298503002/diff/1/sync/engine/commit_util.cc#n...
sync/engine/commit_util.cc:187: AddDefaultFieldValue(meta_entry.GetModelType(),
On 2014/05/19 22:39:05, haitaol1 wrote:
> Can you add a test that calls AddDefaultFieldValue on all data types and
> verifies for the modified specifics:
> 1. No parse error.
> 2. Correct model type can be deducted.
> 
> On 2014/05/19 22:14:57, rlarocque wrote:
> > On 2014/05/19 21:59:12, haitaol1 wrote:
> > > Will it cause parse failure on server if required field in data type
> specifics
> > > is not set?
> > 
> > That's a good point.  It probably would cause a parse failure.  It might
even
> > cause an error earlier, when we try to serialize the incomplete proto.
> > 
> > Fortunately, there are no required fields in specifics.
> > 
> > If there were required fields, I think it would be AddDefaultFieldValue's
> > responsibility to populate them with sensible defaults.  Otherwise, it would
> > likely cause problems elsewhere, too.
> 

Done.

[haitaol1, 2014-05-20 17:24:11]

lgtm

[rlarocque, 2014-05-20 17:30:21]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:31:11]

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

[commit-bot: I haz the power, 2014-05-20 21:42:57]

Change committed as 271763