Invoke AttachmentUploader and update AttachmentIds.

sync/syncable/directory.cc

Index: sync/syncable/directory.cc
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index cebb2d4fd8401cf0410de0bf543e9c89dc55f148..2d8953c4e8f413d7ac0c11c9239430a9e39d22ce 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -458,6 +458,19 @@ void Directory::UpdateAttachmentIndex(
   AddToAttachmentIndex(metahandle, new_metadata, lock);
 }
 
+void Directory::GetMetahandlesByAttachmentId(
+    BaseTransaction* trans,

[maniscalco, 2014/05/09 18:12:11]

I want to confirm my understanding.  The only reason trans is passed here is
that you want to ensure the caller has a transaction, right?

nit: consider DCHECKing trans here as is done elsewhere.

[pavely, 2014/05/13 23:06:44]

Yes, that's right. If unaware developer tries to call Directory without
transaction compiler will complain about it right away.

Actually in most of cases trans is not DCHECKed (look for BaseWriteTransaction
in this file). The reason is that nothing is done with transaction object so it
is only protecting from forgetful developer, not from malicious.

On 2014/05/09 18:12:11, maniscalco wrote:

[maniscalco, 2014/05/15 20:09:28]

Yeah, the only reason I suggesting DCHECKing the transaction is to ensure
someone didn't pass NULL intentionally, believing that transaction is optional. 
Of course, I'm assuming that calling this method when there is no transaction
object on the stack is a bug (no transaction object, no transaction lock
acquisition, right?).

On 2014/05/13 23:06:44, pavely wrote:

+    const sync_pb::AttachmentIdProto& attachment_id_proto,
+    Metahandles* result) {

[maniscalco, 2014/05/09 18:12:11]

nit: consider DCHECKing result.

[pavely, 2014/05/13 23:06:44]

Done.

+  ScopedKernelLock lock(this);
+  IndexByAttachmentId::iterator index_iter =

[maniscalco, 2014/05/09 18:12:11]

Could this be made a const_iterator?

[pavely, 2014/05/13 23:06:44]

Done.

+      kernel_->index_by_attachment_id.find(attachment_id_proto.unique_id());
+  if (index_iter == kernel_->index_by_attachment_id.end())
+    return;

[maniscalco, 2014/05/09 18:12:11]

From the header comment and method name, it's not obvious if a caller should
expect its result to be clear()ed if the attachment id is not found in the
index.  Since this method is GetMetahandlesByAttachmentId and not
AppendMetahandlesByAttachmentId, I would assume it would clear result.

Can you please update the doc to say what happens in this case?

[pavely, 2014/05/13 23:06:44]

Yes, I should have cleared it.
Done.
On 2014/05/09 18:12:11, maniscalco wrote:

+  const MetahandleSet& metahandle_set = index_iter->second;
+  copy(metahandle_set.begin(), metahandle_set.end(), back_inserter(*result));

[maniscalco, 2014/05/09 18:12:11]

Huh, I would have expected this to need std::.  Maybe one of our #includes has a
"using std"...

[pavely, 2014/05/13 23:06:44]

Maybe... I copied it from GetUnsyncedMetaHandles. std::copy is better.
Done.
On 2014/05/09 18:12:11, maniscalco wrote:

+}
+
 bool Directory::unrecoverable_error_set(const BaseTransaction* trans) const {
   DCHECK(trans != NULL);
   return unrecoverable_error_set_;