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

sync/api/attachments/attachment_service_proxy_for_test.cc

+// 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.
+
+#include "sync/api/attachments/attachment_service_proxy_for_test.h"
+
+#include "base/message_loop/message_loop_proxy.h"
+#include "sync/api/attachments/fake_attachment_service.h"
+
+namespace syncer {
+
+AttachmentServiceProxyForTestMember::AttachmentServiceProxyForTestMember(
+    scoped_ptr<AttachmentService> wrapped)
+    : wrapped_(wrapped.Pass()), wrapped_weak_ptr_factory_(wrapped_.get()) {}
+
+AttachmentServiceProxyForTestMember::~AttachmentServiceProxyForTestMember() {}
+
+AttachmentServiceProxyForTest::AttachmentServiceProxyForTest()
+    : AttachmentServiceProxyForTestMember(
+          FakeAttachmentService::CreateForTest()),
+      AttachmentServiceProxy(base::MessageLoopProxy::current(),
+                             wrapped_weak_ptr_factory_.GetWeakPtr()) {}

[tim (not reviewing), 2014/04/04 18:11:34]

Yeah, wow. That's quite the conundrum. I had to go back and rethink ASP's
interface a bit, but I do think it still makes sense. The new thing here is you
want ASPFT to be self contained, which is neat. And this is a cool and some
would say (:p) elegant solution. The problem is it is super weird looking in
chrome code and violates several style guide constraints: no private
inheritance, and no multiple inheritance from non pure virtual interfaces.

You could require tests to add a member that owns the WeakPtrFactory and "wraps"
an ASPFT, so tests would pass aspft_setter_upper_.get() to SyncData.  It's not
as smooth as what you have here though, I agree.

So far I can only think of one other way to achieve the self-containedness of
ASPFT that roughly conforms to chrome conventions and doesn't explicitly violate
style guide: make ASPFT an adapter, where it still extends ASP but uses
componentization and delegation to achieve functionality. It "adapts" a
componentization approach to what SyncData is expecting, which is an ASP.  It's
still a bit fragile, but it's less subtle than what you have here, and seems
kind of at-home in test land, e.g:

class AttachmentServiceProxyForTest : public AttachmentServiceProxy {
 public: 
   AttachmentServiceProxyForTest() : 
       AttachmentServiceProxy(MLP::current(), base::WeakPtr()), 
       service_(FakeAttachmentService::CreateForTest()),
       factory_(service_.get()),
       real_proxy_(MLP::current(), factory_.GetWeakPtr()) {}
  
  // AttachmentServiceProxy implementation.
  virtual void GetOrDownloadAttachments(
       const AttachmentIdList& attachment_ids,
       const GetOrDownloadCallback& callback) OVERRIDE {
    real_proxy_->GetOrDownloadAttachments(attachment_ids, callback);
  }
  ... and so on.

 private:
   AttachmentServiceProxy real_proxy_;
   scoped_ptr<AttachmentService> service_;
   base::WeakPtrFactory<ASPFT> factory_;
}

[maniscalco, 2014/04/04 19:14:23]

Fair enough.  I'm convinced in this case the benefits of a more consistent
code base outweigh the costs of having some extra forwarding functions.

Thanks for the detailed suggestion on the forwarding solution.

Upon closer examination I realize this patch has a slicing problem.
SyncData's ctor copies the ASP.  As the code stands now, this means ASPFTs
will be automatically sliced down to ASPs, causing the ASP's WeakPtr to
become null when the slicing occurs.  An ASP with a null WeakPtr is OK in
the sense that it's not gonna crash or DCHECK, but it's not what I wanted.
My goal was to have a somewhat realistic object graph hanging off the
SyncDatas even in tests.  All the base from member stuff was for naught.

I'll update the patch to fix the slicing problem and provide an ASPFT
with fewer style guide violations.  I may need to update SyncData's ctor
to accept an ASP via scoped_ptr.


On 2014/04/04 18:11:34, timsteele wrote:

[maniscalco, 2014/04/04 22:57:41]

Good news and bad news.

Good news is I can use a static factory function (ASPFT::Create) to work around
the initialization order issue.  I figure it's preferable to the call forwarding
solution because ASPFT won't need to be updated every time ASP's interface
changes.  LMKWYT.

Bad new is that I think AttachmentServiceProxy has to be RefCountedThreadSafe to
avoid the slicing problem.  I cannot make SyncData's AttachmentServiceProxy a
regular scope_ptr because SyncData must be copyable and scoped_ptrs are not. 
I'm reluctant to give up on having a polymorphic ASP in SyndData.  I think it
may come in handy outside of unit tests.  Please LMK if you see an alternative
I've overlooked.

On 2014/04/04 19:14:23, maniscalco wrote:

+
+AttachmentServiceProxyForTest::~AttachmentServiceProxyForTest() {}
+
+}  // namespace syncer