Update Commit and GetUpdatesResponse messages to include attachment ids.

Update Commit and GetUpdatesResponse messages to include attachment ids.

Private function HasAttachmentNotOnServer is now tested (albeit
indirectly) by directory_commit_contribution_unittest.cc via
DirectoryCommitContribution::Build.

When displayed in about:sync, SyncEntity instances will now show any
attachment ids they contain.

This CL depends on issue 395913003.

BUG=356266, 394023
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283908

[maniscalco, 2014-07-15 23:09:08]

Richard, would you please review this change?  This is the second half of issue
386803004 and builds on issue 395913003.

Be sure to take a look at conflict_resolver.cc and
sync/engine/directory_update_handler_unittest.cc as they have both changed from
what they were in the CL you previously looked at (386803004).

I've applied the feedback you left on 386803004.

You brought up the issue of conflict resolution.  I'm not completely sure I've
properly addressed the issue in this CL.  Let's be sure to discuss this
tomorrow.

[rlarocque, 2014-07-16 19:57:23]

I skimmed over this yesterday and it looked pretty good.

I intend to properly review this today, but won't have time to read over it for
the next few hours.

[rlarocque, 2014-07-16 23:00:57]

I think this is OK, though I'll want to double-check with you about the conflict
resolution semantics.

The other thing I should mention, which I should have thought of earlier, is
that you may want to look at SyncManagerImpl::VisiblePropertiesDiffer().  As is,
there's a possibility that attachment-only changes will not be delivered to
their change processors.  I'm not sure whether or not that's a problem.

[maniscalco, 2014-07-16 23:43:42]

On 2014/07/16 23:00:57, rlarocque wrote:
> I think this is OK, though I'll want to double-check with you about the
conflict
> resolution semantics.
> 
> The other thing I should mention, which I should have thought of earlier, is
> that you may want to look at SyncManagerImpl::VisiblePropertiesDiffer().  As
is,
> there's a possibility that attachment-only changes will not be delivered to
> their change processors.  I'm not sure whether or not that's a problem.

Thanks, this is good feedback.

Re conflict resolution semantics, as we talked about offline, we want to ensure
the selected attachment metadata matches the selected entity specifics.  That is
server wins for both attachment metadata and entity specifics or client wins for
both.

Re VisiblePropertiesDiffer, good catch!  Yes, there is almost certainly a bug in
this patch.  I'll updated things and send it back out for review.

[maniscalco, 2014-07-17 16:57:14]

On 2014/07/16 23:43:42, maniscalco wrote:
> On 2014/07/16 23:00:57, rlarocque wrote:
> > I think this is OK, though I'll want to double-check with you about the
> conflict
> > resolution semantics.
> > 
> > The other thing I should mention, which I should have thought of earlier, is
> > that you may want to look at SyncManagerImpl::VisiblePropertiesDiffer().  As
> is,
> > there's a possibility that attachment-only changes will not be delivered to
> > their change processors.  I'm not sure whether or not that's a problem.
> 
> Thanks, this is good feedback.
> 
> Re conflict resolution semantics, as we talked about offline, we want to
ensure
> the selected attachment metadata matches the selected entity specifics.  That
is
> server wins for both attachment metadata and entity specifics or client wins
for
> both.
> 
> Re VisiblePropertiesDiffer, good catch!  Yes, there is almost certainly a bug
in
> this patch.  I'll updated things and send it back out for review.

RFAL!  I've uploaded a patch set that updates VisiblePropertiesDiffer and adds a
test case.

[rlarocque, 2014-07-17 17:22:53]

On 2014/07/17 16:57:14, maniscalco wrote:
> On 2014/07/16 23:43:42, maniscalco wrote:
> > On 2014/07/16 23:00:57, rlarocque wrote:
> > > I think this is OK, though I'll want to double-check with you about the
> > conflict
> > > resolution semantics.
> > > 
> > > The other thing I should mention, which I should have thought of earlier,
is
> > > that you may want to look at SyncManagerImpl::VisiblePropertiesDiffer(). 
As
> > is,
> > > there's a possibility that attachment-only changes will not be delivered
to
> > > their change processors.  I'm not sure whether or not that's a problem.
> > 
> > Thanks, this is good feedback.
> > 
> > Re conflict resolution semantics, as we talked about offline, we want to
> ensure
> > the selected attachment metadata matches the selected entity specifics. 
That
> is
> > server wins for both attachment metadata and entity specifics or client wins
> for
> > both.
> > 
> > Re VisiblePropertiesDiffer, good catch!  Yes, there is almost certainly a
bug
> in
> > this patch.  I'll updated things and send it back out for review.
> 
> RFAL!  I've uploaded a patch set that updates VisiblePropertiesDiffer and adds
a
> test case.

LGTM.

[maniscalco, 2014-07-17 17:23:17]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-07-17 17:25:05]

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

[commit-bot: I haz the power, 2014-07-17 22:29:20]

Change committed as 283908