Keep track of which attachments are referenced by which sync entries.

Keep track of which attachments are referenced by which sync entries.

PutAttachmentMetadata on MutableEntry now notifies the Directory when
the attachments associated with an entry change.

Give Directory::InitializeIndices a ScopedKernelLock to be consistent
and make it easier to ensure we are locking when we need to.

GenericChangeProcess passes new attachments to AttachmentService for
storage and upload.

Add an output parameter to GenericChangeProcessor's HandleActionAdd and
HandleActionUpdate methods so they can keep track of potentially new
attachments and pass them to AttachmentService for storage/upload.

Convert AttachmentService's OnSyncDataAdd to StoreAttachments.

Implement HasAttachmentNotOnServer.

BUG=348625, 353303, 354530, 356266
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267617

[maniscalco, 2014-04-22 19:49:55]

Pavel, would you please review this change?

As I mentioned earlier today, I want to get this out so you can start reviewing
while I working adding some test cases.  Looks like syncer_unittest.cc is
probably the place to add some.

Tim, FYI.

[pavely, 2014-04-28 23:26:27]

https://codereview.chromium.org/247983002/diff/60001/components/sync_driver/g...
File components/sync_driver/generic_change_processor.cc (right):

https://codereview.chromium.org/247983002/diff/60001/components/sync_driver/g...
components/sync_driver/generic_change_processor.cc:344: syncer::ModelType type,
nit: Indentation is off.

https://codereview.chromium.org/247983002/diff/60001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/247983002/diff/60001/sync/syncable/directory....
sync/syncable/directory.cc:426: iter->second.erase(metahandle);
Can entity reference attachment multiple times? Seems like there is noting in
the code to prevent that.

[maniscalco, 2014-04-29 20:53:21]

Thanks for the feedback!  All applied.  RFAL.

https://codereview.chromium.org/247983002/diff/60001/components/sync_driver/g...
File components/sync_driver/generic_change_processor.cc (right):

https://codereview.chromium.org/247983002/diff/60001/components/sync_driver/g...
components/sync_driver/generic_change_processor.cc:344: syncer::ModelType type,
On 2014/04/28 23:26:27, pavely wrote:
> nit: Indentation is off.

Fixed (and ran through "git cl format").

https://codereview.chromium.org/247983002/diff/60001/sync/syncable/directory.cc
File sync/syncable/directory.cc (right):

https://codereview.chromium.org/247983002/diff/60001/sync/syncable/directory....
sync/syncable/directory.cc:426: iter->second.erase(metahandle);
On 2014/04/28 23:26:27, pavely wrote:
> Can entity reference attachment multiple times? Seems like there is noting in
> the code to prevent that.

Good point.  No, a single SyncData (or entry) should not refer to the same
attachment twice.

I've added a DCHECK to SyncData::CreateLocalDataWithAttachments to catch this
case.  Of course, because it's a DCHECK it won't fire in release builds.  A
stronger, but slightly more expensive option would be to skip DCHECKing and
instead just remove the duplicates (std::sort(); std::unique();).  I don't
anticipate more than 10 or 20 attachments for an entry so I would happy to go
with the slightly more expensive option.  WDYT?

[pavely, 2014-04-30 20:22:11]

One comment, otherwise lgtm.

https://codereview.chromium.org/247983002/diff/140001/sync/syncable/DEPS
File sync/syncable/DEPS (right):

https://codereview.chromium.org/247983002/diff/140001/sync/syncable/DEPS#newc...
sync/syncable/DEPS:6: "+sync/internal_api/public",
Could you take a look at comments between me and Tim about similar change:

https://codereview.chromium.org/248473002/diff/1/sync/engine/DEPS

https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...

[maniscalco, 2014-05-01 00:34:15]

Thanks for the feedback.  RFAL!

https://codereview.chromium.org/247983002/diff/140001/sync/syncable/DEPS
File sync/syncable/DEPS (right):

https://codereview.chromium.org/247983002/diff/140001/sync/syncable/DEPS#newc...
sync/syncable/DEPS:6: "+sync/internal_api/public",
On 2014/04/30 20:22:12, pavely wrote:
> Could you take a look at comments between me and Tim about similar change:
> 
> https://codereview.chromium.org/248473002/diff/1/sync/engine/DEPS
> 
>
https://codereview.chromium.org/250943003/diff/1/sync/internal_api/public/bas...

Thanks for pointing this out!  I had a conversation with Tim about DEPS.  Adding
these news allowances would have been a mistake so I've done three things to
rectify the situation.

1. I've moved the AttachmentIdProto initialization out of sync/api and into
sync/internal_api/base where sync/syncable can depend on it.  Directory and
friends have been updated to reflect this move.

2. I've moved the AttachmentLinking test case to sync_manager_impl_unittest.cc,
which is an arguably better home for it since it used the internal_api layer,
not the syncable layer.

3. I've added more test cases to directory_unittest.cc that test  Directory
using the syncable layer, which fits with the other test cases in the file.

[tim (not reviewing), 2014-05-01 18:42:13]

LGTM

[maniscalco, 2014-05-01 18:44:37]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-05-01 18:44:55]

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

[commit-bot: I haz the power, 2014-05-01 20:25:12]

Change committed as 267617

[maniscalco, 2014-05-01 21:58:53]

A revert of this CL has been created in
https://codereview.chromium.org/265853004/ by maniscalco@chromium.org.

The reason for reverting is: Looks like this may have introduced a memory leak:

http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...

Reverting.  Will investigate and reland when appropriate..