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

Replace deprecated 3-arg SyncData::CreateLocalData with 5-arg version.

Update unit tests that call SyncData::CreateLocalData to call the new
version.

Add AttachmentServiceProxyForTest to reduce boilerplate in tests.

Add a MessageLoop to ManagedUserSyncServiceTest, DomDistillerStoreTest,
and SyncChangeTest because AttachmentServiceProxyForTest needs one.

Format modified lines with clang-format.

TBR=pam,droger,asargent,pkasting,pkotwicz,cjhopman

BUG=353296
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263128

[maniscalco, 2014-04-01 20:04:27]

Hey Tim, mind reviewing this change?

This is a follow up to the sync API change.  It's mostly tedious and boring,
except for sync/api/attachments/attachment_service_proxy_for_test.*

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

https://codereview.chromium.org/213003004/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy_for_test.cc (right):

https://codereview.chromium.org/213003004/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy_for_test.cc:22:
wrapped_weak_ptr_factory_.GetWeakPtr()) {}
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:22]

https://codereview.chromium.org/213003004/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy_for_test.cc (right):

https://codereview.chromium.org/213003004/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy_for_test.cc:22:
wrapped_weak_ptr_factory_.GetWeakPtr()) {}
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:
> 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 22:57:40]

PTAL!

https://codereview.chromium.org/213003004/diff/120001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy_for_test.cc (right):

https://codereview.chromium.org/213003004/diff/120001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy_for_test.cc:22:
wrapped_weak_ptr_factory_.GetWeakPtr()) {}
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:
> 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:
> > 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_;
> > }
> > 
>

[tim (not reviewing), 2014-04-07 20:23:10]

https://codereview.chromium.org/213003004/diff/180001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy_for_test.h (right):

https://codereview.chromium.org/213003004/diff/180001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy_for_test.h:22: static
scoped_refptr<AttachmentServiceProxyForTest> Create();
I see.  Sad face. In this case I still think the more common thing would be to
have only a 'Core' object be RCTS like WeakHandle, or URLFetcherImpl, or
SyncBackendHostImpl. We tend to bury ref-counting as low as possible in chrome
code. I mentioned this on https://codereview.chromium.org/187303006/ but I said
to ignore it in favor of the solution we thought was going to work at the time
but now we see is fragile when testing due to truncation.

What do you think?

[maniscalco, 2014-04-08 17:51:56]

RFAL!

https://codereview.chromium.org/213003004/diff/180001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy_for_test.h (right):

https://codereview.chromium.org/213003004/diff/180001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy_for_test.h:22: static
scoped_refptr<AttachmentServiceProxyForTest> Create();
On 2014/04/07 20:23:10, timsteele wrote:
> I see.  Sad face. In this case I still think the more common thing would be to
> have only a 'Core' object be RCTS like WeakHandle, or URLFetcherImpl, or
> SyncBackendHostImpl. We tend to bury ref-counting as low as possible in chrome
> code. I mentioned this on https://codereview.chromium.org/187303006/ but I
said
> to ignore it in favor of the solution we thought was going to work at the time
> but now we see is fragile when testing due to truncation.
> 
> What do you think?

I like this approach.  Done.  ASP is no longer RCTS.

https://codereview.chromium.org/213003004/diff/200001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy_for_test.h (right):

https://codereview.chromium.org/213003004/diff/200001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy_for_test.h:20: : public
AttachmentServiceProxy {
One approach I considered, but didn't implement was to replace this class with a
namespace.  The downside was that I'd need to make public ASP::ForwardingCore
and ASP ctor that accepts a ForwardingCore.  Either public or use friend.  I
figured this approach reduces the exposure.  LMK if you prefer the other
approach.

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

LGTM with nits.

https://codereview.chromium.org/213003004/diff/220001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy.h (right):

https://codereview.chromium.org/213003004/diff/220001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:77: class SYNC_EXPORT
ForwardingCore
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.

https://codereview.chromium.org/213003004/diff/220001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:104: 
comment this is for tests.

[maniscalco, 2014-04-10 20:00:43]

All applied.  Thanks for the review.

https://codereview.chromium.org/213003004/diff/220001/sync/api/attachments/at...
File sync/api/attachments/attachment_service_proxy.h (right):

https://codereview.chromium.org/213003004/diff/220001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:77: class SYNC_EXPORT
ForwardingCore
On 2014/04/10 18:21:41, timsteele wrote:
> 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.
> 

Rename ForwardingCore to Core: Done.

Opening comment for Core: Done.

Comment re pass by const-ref: Done.

Comment re AS to ASP: Done.

https://codereview.chromium.org/213003004/diff/220001/sync/api/attachments/at...
sync/api/attachments/attachment_service_proxy.h:104: 
On 2014/04/10 18:21:41, timsteele wrote:
> comment this is for tests.

Done.

[maniscalco, 2014-04-10 20:25:33]

Adding OWNERS of downstream unit tests as TBR reviewers.

[maniscalco, 2014-04-10 20:26:42]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-10 20:26:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/213003004/240001

[maniscalco, 2014-04-10 20:48:19]

Adding OWNERS of downstream unit tests as TBR reviewers (for real this time).

[commit-bot: I haz the power, 2014-04-10 21:44:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-10 21:44:20]

List of reviewers changed.
droger@chromium.org,pkasting@chromium.org,pam@chromium.org,cjhopman@chromium....
did a drive-by without LGTM'ing!

[maniscalco, 2014-04-10 22:29:15]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-04-10 22:30:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/213003004/240001

[commit-bot: I haz the power, 2014-04-10 23:40:16]

Change committed as 263128

[Peter Kasting, 2014-04-14 22:48:05]

On 2014/04/10 20:25:33, maniscalco wrote:
> Adding OWNERS of downstream unit tests as TBR reviewers.

Please always say when you do this who you're adding, and what you want each
person to review.

There are a lot of reviewers on this change now, and I don't know which parts I
(or anyone else) am supposed to TBR.

[maniscalco, 2014-04-15 16:08:52]

Thanks, Peter.  Sorry about omitting the list.

TBR folks would you please review the following files:

asargent@chromium.org:
chrome/browser/extensions/api/sessions/sessions_apitest.cc 

brettw@chromium.org:
chrome/browser/history/history_unittest.cc

pam@chromium.org:
chrome/browser/managed_mode/managed_user_registration_utility_unittest.cc
chrome/browser/managed_mode/managed_user_sync_service_unittest.cc
chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc

pkotwicz@chromium.org:
chrome/browser/themes/theme_syncable_service_unittest.cc

pkasting@chromium.org:
chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc

cjhopman@chromium.org:
components/dom_distiller/core/dom_distiller_store_unittest.cc

[Peter Kasting, 2014-04-15 19:56:51]

LGTM