Invoke AttachmentUploader and update AttachmentIds.

sync/syncable/directory.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/syncable/directory.h"
 
 #include <iterator>
 
 #include "base/base64.h"
 #include "base/debug/trace_event.h"
@@ skipping 440 matching lines @@
 
 void Directory::UpdateAttachmentIndex(
     const int64 metahandle,
     const sync_pb::AttachmentMetadata& old_metadata,
     const sync_pb::AttachmentMetadata& new_metadata) {
   ScopedKernelLock lock(this);
   RemoveFromAttachmentIndex(metahandle, old_metadata, lock);
   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_;
 }
 
 void Directory::ClearDirtyMetahandles() {
   kernel_->transaction_mutex.AssertAcquired();
   kernel_->dirty_metahandles.clear();
 }
 
@@ skipping 951 matching lines @@
 
   for (OrderedChildSet::const_iterator i = children->begin();
        i != children->end(); ++i) {
     DCHECK_EQ(parent_id, (*i)->ref(PARENT_ID));
     result->push_back((*i)->ref(META_HANDLE));
   }
 }
 
 }  // namespace syncable
 }  // namespace syncer