Refactor SyncBackendHost

Refactor SyncBackendHost

Split SyncBackendHost into SyncBackendHost, SyncBackendHostImpl and
SyncBackendHostCore.

The SyncBackendHost class is now a pure virtual interface.  It is the
contains the interface exposed to the ProfileSyncService.

The SyncBackendHostImpl class implements the functionality of the
SyncBackendHost.  It contains most of the code that used to reside in
SyncBackendHost.

The SyncBackendHostCore is the old SyncBackendHost::Core under a new
name.  It's been moved out of the SyncBackendHost's .cc file and into
its own, publicly declared class.  This will hopefully enable better
testing in the future.

This is a pure refactoring change.  It should have no impact on program
behavior.

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

[rlarocque, 2013-11-06 16:10:27]

Please review.

[Nicolas Zea, 2013-11-06 18:03:20]

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:40: // An API to "host" the top
level syncapi element.
nit: syncapi -> SyncAPI

Also , typically the interface itself is the class that documents what the
virtual methods are all supposed to be doing. I think I prefer that approach,
which then allows you to let the impl just have a comment saying
"SyncBackendHost implementation" for all the overridden elements.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:48: virtual ~SyncBackendHost() = 0;
This shouldn't be purely virtual

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_core.cc:42: namespace syncer {
nit: I think we typically have forward declarations above anon namespace values

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_core.h:22: struct DoInitializeOptions
{
can forward declare this I think

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_core.h:70: struct
DoConfigureSyncerTypes {
this too

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:16: #include
"chrome/browser/sync/glue/sync_backend_host_impl.h"
nit: move to be first #include?

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_frontend.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_frontend.h:110: virtual ~SyncFrontend();
I don't think this is really necessary (although I know that's how it was
before).

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_frontend.h:113:
DISALLOW_COPY_AND_ASSIGN(SyncFrontend);
I think typically we leave the DISALLOW_COPY_AND_ASSIGN in implementation right?

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:11: #include
"chrome/browser/sync/glue/sync_backend_host_core.h"
is this necessary?

https://codereview.chromium.org/61183003/diff/50001/sync/internal_api/public/...
File sync/internal_api/public/sync_manager.h (right):

https://codereview.chromium.org/61183003/diff/50001/sync/internal_api/public/...
sync/internal_api/public/sync_manager.h:23: #include
"sync/internal_api/public/internal_components_factory.h"
don't think this is necessary?

[rlarocque, 2013-11-06 21:48:47]

The latest patch fixes some of the issues.  We should talk about the rest.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:40: // An API to "host" the top
level syncapi element.
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> nit: syncapi -> SyncAPI
> 
> Also , typically the interface itself is the class that documents what the
> virtual methods are all supposed to be doing. I think I prefer that approach,
> which then allows you to let the impl just have a comment saying
> "SyncBackendHost implementation" for all the overridden elements.

Done.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:48: virtual ~SyncBackendHost() = 0;
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> This shouldn't be purely virtual

Why?

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_core.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_core.cc:42: namespace syncer {
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> nit: I think we typically have forward declarations above anon namespace
values

Done.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_core.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_core.h:22: struct DoInitializeOptions
{
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> can forward declare this I think

Yes and no.

This could be forward declared, but these definitions need to be put in a .h
file somewhere.  The right solution might be to put it in
do_initialize_options.h.  However, I think these definitions are special.

These structs exist because we've reached the limit of the number of parameters
we can bind to a function.  They're part of an extended function definition.  It
makes sense to have them defined in the same location as the functions they help
define.  That way, the functions that want to call DoInitialize() can get
everything they need (SyncBackendHostCore::DoInitialize and DoInitializeOptions)
just by including this header.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:16: #include
"chrome/browser/sync/glue/sync_backend_host_impl.h"
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> nit: move to be first #include?

The style checker complains.  I think it has an exception that allows foo.h to
be the first include in foo_unittest.cc.  Since the name changed, the exception
no longer applies.

I could rename this file to sync_backend_host_impl_unittest.cc then move it
back.  I was on the fence about doing that anyway.  I figured that since
SyncBackendHostImpl is the only "real" SyncBackendHost, it made sense to keep
calling its tests sync_backend_host_unittest.cc.  I could go either way, though.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_frontend.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_frontend.h:110: virtual ~SyncFrontend();
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> I don't think this is really necessary (although I know that's how it was
> before). 

I don't see any harm in it.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_frontend.h:113:
DISALLOW_COPY_AND_ASSIGN(SyncFrontend);
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> I think typically we leave the DISALLOW_COPY_AND_ASSIGN in implementation
right?

Done.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/test_...
File chrome/browser/sync/test_profile_sync_service.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/test_...
chrome/browser/sync/test_profile_sync_service.cc:11: #include
"chrome/browser/sync/glue/sync_backend_host_core.h"
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> is this necessary?

Yeah, for DoInitializeOptions.  This may be an argument for pulling that
definition into its own file.

https://codereview.chromium.org/61183003/diff/50001/sync/internal_api/public/...
File sync/internal_api/public/sync_manager.h (right):

https://codereview.chromium.org/61183003/diff/50001/sync/internal_api/public/...
sync/internal_api/public/sync_manager.h:23: #include
"sync/internal_api/public/internal_components_factory.h"
On 2013/11/06 18:03:21, Nicolas Zea wrote:
> don't think this is necessary?

Done.

[Nicolas Zea, 2013-11-06 22:18:24]

Btw, the unit test appears to have fallen out of the patch? Forget to git add?

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:48: virtual ~SyncBackendHost() = 0;
On 2013/11/06 21:48:47, rlarocque wrote:
> On 2013/11/06 18:03:21, Nicolas Zea wrote:
> > This shouldn't be purely virtual
> 
> Why?

Because it's kind of confusing and against the style guide. A purely virtual
destructor doesn't really mean much. You still have to define it. And our style
guide already requires all destructors to be virtual.

See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:16: #include
"chrome/browser/sync/glue/sync_backend_host_impl.h"
On 2013/11/06 21:48:47, rlarocque wrote:
> On 2013/11/06 18:03:21, Nicolas Zea wrote:
> > nit: move to be first #include?
> 
> The style checker complains.  I think it has an exception that allows foo.h to
> be the first include in foo_unittest.cc.  Since the name changed, the
exception
> no longer applies.
> 
> I could rename this file to sync_backend_host_impl_unittest.cc then move it
> back.  I was on the fence about doing that anyway.  I figured that since
> SyncBackendHostImpl is the only "real" SyncBackendHost, it made sense to keep
> calling its tests sync_backend_host_unittest.cc.  I could go either way,
though.

Ah, yeah, I'd prefer renaming to match the impl

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_frontend.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_frontend.h:110: virtual ~SyncFrontend();
On 2013/11/06 21:48:47, rlarocque wrote:
> On 2013/11/06 18:03:21, Nicolas Zea wrote:
> > I don't think this is really necessary (although I know that's how it was
> > before). 
> 
> I don't see any harm in it.

Just to keep things consistent with other interfaces. It's weird having this
down here. I don't feel too strongly.

[rlarocque, 2013-11-06 22:48:47]

Patch updated.  I've fixed the upload, too.  Turns out I had the wrong upstream
branch set.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host.h:48: virtual ~SyncBackendHost() = 0;
On 2013/11/06 22:18:25, Nicolas Zea wrote:
> On 2013/11/06 21:48:47, rlarocque wrote:
> > On 2013/11/06 18:03:21, Nicolas Zea wrote:
> > > This shouldn't be purely virtual
> > 
> > Why?
> 
> Because it's kind of confusing and against the style guide. A purely virtual
> destructor doesn't really mean much. You still have to define it. And our
style
> guide already requires all destructors to be virtual.
> 
> See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Interfaces

OK.  Fixed.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:16: #include
"chrome/browser/sync/glue/sync_backend_host_impl.h"
On 2013/11/06 22:18:25, Nicolas Zea wrote:
> On 2013/11/06 21:48:47, rlarocque wrote:
> > On 2013/11/06 18:03:21, Nicolas Zea wrote:
> > > nit: move to be first #include?
> > 
> > The style checker complains.  I think it has an exception that allows foo.h
to
> > be the first include in foo_unittest.cc.  Since the name changed, the
> exception
> > no longer applies.
> > 
> > I could rename this file to sync_backend_host_impl_unittest.cc then move it
> > back.  I was on the fence about doing that anyway.  I figured that since
> > SyncBackendHostImpl is the only "real" SyncBackendHost, it made sense to
keep
> > calling its tests sync_backend_host_unittest.cc.  I could go either way,
> though.
> 
> Ah, yeah, I'd prefer renaming to match the impl

Done.

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/sync_frontend.h (right):

https://codereview.chromium.org/61183003/diff/50001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/sync_frontend.h:110: virtual ~SyncFrontend();
On 2013/11/06 22:18:25, Nicolas Zea wrote:
> On 2013/11/06 21:48:47, rlarocque wrote:
> > On 2013/11/06 18:03:21, Nicolas Zea wrote:
> > > I don't think this is really necessary (although I know that's how it was
> > > before). 
> > 
> > I don't see any harm in it.
> 
> Just to keep things consistent with other interfaces. It's weird having this
> down here. I don't feel too strongly.

OK.  I'll remove it.

I was hesitant before, because I thought the person who wrote it might know some
obscure bit of C++ trivia that I don't.  Since no one else can figure out why
this is necessary, I guess it's safe to remove it.

[Nicolas Zea, 2013-11-06 22:53:54]

lgtm

[commit-bot: I haz the power, 2013-11-06 23:13:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/61183003/250001

[commit-bot: I haz the power, 2013-11-07 07:50:00]

Change committed as 233533