Replace calls to 3-arg SyncData::CreateLocalData with 5-arg version.

sync/api/attachments/attachment_service_proxy.h

 // Copyright 2014 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.
 
 #ifndef SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_PROXY_H_
 #define SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_PROXY_H_
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequenced_task_runner.h"
 #include "base/task_runner.h"
 #include "sync/api/attachments/attachment.h"
 #include "sync/api/attachments/attachment_service.h"
 #include "sync/base/sync_export.h"
 
 namespace syncer {
 
 class SyncData;
 
 // AttachmentServiceProxy wraps an AttachmentService allowing multiple threads
-// to share the wrapped AttachmentService.
+// to share the wrapped AttachmentService and invoke its methods in the
+// appropriate thread.
 //
 // Callbacks passed to methods on this class will be invoked in the same thread
 // from which the method was called.
 //
 // This class does not own its wrapped AttachmentService object.  This class
 // holds a WeakPtr to the wrapped object.  Once the the wrapped object is
 // destroyed, method calls on this object will be no-ops.
 //
-//  Users of this class should take care to destroy the wrapped object on the
-//  correct thread (wrapped_task_runner).
+// Users of this class should take care to destroy the wrapped object on the
+// correct thread (wrapped_task_runner).
 //
 // This class is thread-safe.
 class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService {
  public:
   // Default copy and assignment are welcome.
 
   // Construct an invalid AttachmentServiceProxy.
   AttachmentServiceProxy();
 
   // Construct an AttachmentServiceProxy that forwards calls to |wrapped| on the
   // |wrapped_task_runner| thread.
+  //
+  // Note, this object does not own |wrapped|.  When |wrapped| is destroyed,
+  // calls to this object become no-ops.
   AttachmentServiceProxy(
       const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
-      base::WeakPtr<syncer::AttachmentService> wrapped);
+      const base::WeakPtr<syncer::AttachmentService>& wrapped);
 
   virtual ~AttachmentServiceProxy();
 
   // AttachmentService implementation.
   virtual void GetOrDownloadAttachments(const AttachmentIdList& attachment_ids,
                                         const GetOrDownloadCallback& callback)
       OVERRIDE;
   virtual void DropAttachments(const AttachmentIdList& attachment_ids,
                                const DropCallback& callback) OVERRIDE;
   virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
   virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
   virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
                                 const SyncData& updated_sync_data) OVERRIDE;
 
+ protected:
+  // A ref-counted AttachmentService that forwards to a wrapped
+  // AttachmentService.
+  //
+  // This class is ref-counted because we want to allow AttachmentServiceProxy
+  // to be copy-constructable while allowing for different implementations of
+  // ForwardingCore (e.g. one type of core might own the wrapped
+  // AttachmentService).
+  //
+  // Calls to objects of this class become no-ops once its wrapped object is
+  // destroyed.
+  class SYNC_EXPORT ForwardingCore

[tim (not reviewing), 2014/04/10 18:21:41]

nit - I'd rather just call this Core.

Also, I think the opening comment for this should be "Core does the real work of
proxying across threads so that AttachmentServiceProxy is an easy-to-use,
ref-counting-free object."

I suppose it's slightly better to pass ASPs by const-ref than pass-by-copy to
avoid AddRef/Release... which might be worth a mention at the ASP comment
level.. but not a big deal here.

Lastly, because you're explicitly choosing to stick with static functions for
communication in the the AS -> ASP direction, it might help understand the
implementation if you call out that Core is only used for proxying requests
*from* ASP *to* AS.  Or, alternatively, move the proxyCallback methods to
ASP::Core so it's the same both ways.

[maniscalco, 2014/04/10 20:00:44]

Rename ForwardingCore to Core: Done.

Opening comment for Core: Done.

Comment re pass by const-ref: Done.

Comment re AS to ASP: Done.

+      : public AttachmentService,
+        public base::RefCountedThreadSafe<ForwardingCore> {
+   public:
+    // Construct an AttachmentServiceProxyCore that forwards calls to |wrapped|.
+    ForwardingCore(const base::WeakPtr<syncer::AttachmentService>& wrapped);
+
+    // AttachmentService implementation.
+    virtual void GetOrDownloadAttachments(
+        const AttachmentIdList& attachment_ids,
+        const GetOrDownloadCallback& callback) OVERRIDE;
+    virtual void DropAttachments(const AttachmentIdList& attachment_ids,
+                                 const DropCallback& callback) OVERRIDE;
+    virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE;
+    virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE;
+    virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids,
+                                  const SyncData& updated_sync_data) OVERRIDE;
+
+   protected:
+    friend class base::RefCountedThreadSafe<ForwardingCore>;
+    virtual ~ForwardingCore();
+
+   private:
+    base::WeakPtr<AttachmentService> wrapped_;
+
+    DISALLOW_COPY_AND_ASSIGN(ForwardingCore);
+  };
+

[tim (not reviewing), 2014/04/10 18:21:41]

comment this is for tests.

[maniscalco, 2014/04/10 20:00:44]

Done.

+  AttachmentServiceProxy(
+      const scoped_refptr<base::SequencedTaskRunner>& wrapped_task_runner,
+      const scoped_refptr<ForwardingCore>& core);
+
  private:
   scoped_refptr<base::SequencedTaskRunner> wrapped_task_runner_;
-  // wrapped_ must only be dereferenced on the wrapped_task_runner_ thread.
-  base::WeakPtr<AttachmentService> wrapped_;
+  scoped_refptr<ForwardingCore> core_;
 };
 
 }  // namespace syncer
 
 #endif  // SYNC_API_ATTACHMENTS_ATTACHMENT_SERVICE_PROXY_H_