Invoke AttachmentUploader and update AttachmentIds.

sync/api/attachments/attachment_service.h

Index: sync/api/attachments/attachment_service.h
diff --git a/sync/api/attachments/attachment_service.h b/sync/api/attachments/attachment_service.h
index 9e5e25d1489afef08909549afa08ed1f36e74cc9..c9c4b3337259901ee826126cb0fc3c55f73d4123 100644
--- a/sync/api/attachments/attachment_service.h
+++ b/sync/api/attachments/attachment_service.h
@@ -54,9 +54,23 @@ class SYNC_EXPORT AttachmentService {
 
   typedef base::Callback<void(const StoreResult&)> StoreCallback;
 
+  // An interface that embedder code implements to be notified about different
+  // events that originate from AttachmentService.
+  class Delegate {

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

I'm thinking about in which thread the delegate will be called.  It probably
deserves a comment that it will be called in the same thread from which the
AttachmentService was called.

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

Done.

+   public:
+    virtual ~Delegate() {}
+
+    // Attachment is uploaded to server and attachment_id is updated with server
+    // url.
+    virtual void OnAttachmentUploaded(const AttachmentId& attachment_id) = 0;
+  };
+
   AttachmentService();
   virtual ~AttachmentService();
 
+  // Set delegate pointer. AttachmentService doesn't own delegate.

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

Can we avoid adding a no-op implementation here?  By putting this setter on the
interface (AttachmentService) we're telling the world that every
AttachmentService is capable of getting a delegate, but that's not really true
(e.g. AttachmentServiceProxy).  Let's chat about this one.  I see that we wan't
just pass the delegate to the ctor of the derived type because GCP *is* the
delegate so it's not immediately obvious to me how we can accomplish this.

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

The reason was to not add implementation of it to AttachmentServiceProxy , but
you are right, I'd rather add NOTREACHED there.

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

+  virtual void SetDelegate(Delegate* delegate) {}

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

In the future we may move the delegate functionality out of GCP at which point
the AS may outlive its delegate.  I suggest updating AS to take a Delegate via
WeakPtr.

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

Right now interface simple, explicit and will blow up if developer does
something wrong. It is similar to AddObserver/RemoveObserver pattern:
AttachmentService already exists when GCP is created, GCP sets itself as
delegate at init and removes when it goes away.

WeakPtr on the other hand offer pattern where mistakes go unnoticed. It will
silently work without reporting any errors, only sometimes (maybe often) it
won't update IDs after upload and we wouldn't even know it.

I'd say let's keep it simple for now and change it to WeakPtr when we have case
for it.
On 2014/05/09 18:12:11, maniscalco wrote:

+
   // See SyncData::GetOrDownloadAttachments.
   virtual void GetOrDownloadAttachments(
       const AttachmentIdList& attachment_ids,