[Sync] Componentize ProfileSyncComponentsFactoryImpl

[Sync] Componentize ProfileSyncComponentsFactoryImpl

Componentize ProfileSyncComponentsFactoryImpl into browser_sync component.
As part of this, pull out RegisterDesktopDatatypes method into ChromeSyncClient.
Similarly, a RegisterAndroidDataTypes is created instead for Android
platforms.

Because profile_sync_components_factory_impl_unittest.cc was really duplicating
and testing the logic from ProfileSyncServiceFactory, went ahead and moved the
test cases over into profile_sync_service_factory_unittest.cc.

BUG=512768
TBR=rdevlin.cronin@chromium.org

Committed: https://crrev.com/7db382299ca33ffb369f9c79a5331bbfbab259bc
Cr-Commit-Position: refs/heads/master@{#359916}

[Nicolas Zea, 2015-11-09 21:52:47]

zea@chromium.org changed reviewers:
+ blundell@chromium.org

[Nicolas Zea, 2015-11-09 21:52:47]

+Colin, PTAL

[blundell, 2015-11-10 15:24:25]

Looks good! Mostly nits, with one bigger question about the unittest.

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
chrome/browser/sync/chrome_sync_client.cc:456: void
ChromeSyncClient::RegisterDesktopDataTypes(
Why not just have this be an instance method and then not have to have the
SyncClient instance as an argument to the callback?

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
File chrome/browser/sync/chrome_sync_client.h (right):

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
chrome/browser/sync/chrome_sync_client.h:59: // being explicitly
enabled/disabled by the command line.
should this be "explicitly disabled/enabled"? I realize the comment was this way
before as well but I don't understand it :).

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc
(left):

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc:7: #include
"base/command_line.h"
Naively it seems like we could keep this unittest and have it just test the
//components-level functionality, i.e., strip out all the stuff that is now
specific to the callback passed in by ChromeSyncClient. We could move just the
tests of the functionality that is specific into that callback into
profile_sync_service_factory_unittest. Is that doable/worth having?

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_components_factory_impl.cc
(left):

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:547:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
nit: Why not keep this DCHECK?

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_components_factory_impl.cc
(right):

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:391: #if
defined(OS_ANDROID)
Could you add || defined(OS_IOS) here?

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.h:32: //
types that or on or off by default (respectively).
or on -> are on

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.h:116:
const std::string history_disabled_pref_;
nit: So far we've been keeping this as a const char*. Seems like it's worth
having consistency?

[blundell, 2015-11-10 16:10:49]

I'm OOO for a French holiday tomorrow, btw.

[Nicolas Zea, 2015-11-11 00:06:20]

PTAL!

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
chrome/browser/sync/chrome_sync_client.cc:456: void
ChromeSyncClient::RegisterDesktopDataTypes(
On 2015/11/10 15:24:24, blundell wrote:
> Why not just have this be an instance method and then not have to have the
> SyncClient instance as an argument to the callback?

Originally, the SyncAPIComponentFactory itself was an argument to the
ChromeSyncClient at construction time. So I needed to be able to create the
factory first, including passing this method through at the factory's
construction time.

That said, you're right that this is a bit weird. I've changed it up now to have
the SyncClient's Initialize method include the SyncApiComponentFactory, which
should resolve this (although required refactoring the initialization of the
sync client, factory, and the PSS).

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
File chrome/browser/sync/chrome_sync_client.h (right):

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/chrome_...
chrome/browser/sync/chrome_sync_client.h:59: // being explicitly
enabled/disabled by the command line.
On 2015/11/10 15:24:24, blundell wrote:
> should this be "explicitly disabled/enabled"? I realize the comment was this
way
> before as well but I don't understand it :).

Done.

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc
(left):

https://codereview.chromium.org/1421003007/diff/1/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_components_factory_impl_unittest.cc:7: #include
"base/command_line.h"
On 2015/11/10 15:24:24, blundell wrote:
> Naively it seems like we could keep this unittest and have it just test the
> //components-level functionality, i.e., strip out all the stuff that is now
> specific to the callback passed in by ChromeSyncClient. We could move just the
> tests of the functionality that is specific into that callback into
> profile_sync_service_factory_unittest. Is that doable/worth having?

The way this test is written right now, it's really testing the component
factory's interaction with the PSS and the sync client, which really depends on
the ProfileSyncServiceFactory initializing them properly. We could write new
unit tests specifically for the component factory impl, but given the nature of
this factory, and the fact that it doesn't have much real logic in it, I'm not
sure how much that's worth it with this testing in place.

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_components_factory_impl.cc
(left):

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:547:
DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2015/11/10 15:24:24, blundell wrote:
> nit: Why not keep this DCHECK?

Done.

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_components_factory_impl.cc
(right):

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.cc:391: #if
defined(OS_ANDROID)
On 2015/11/10 15:24:24, blundell wrote:
> Could you add || defined(OS_IOS) here?

Done.

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.h:32: //
types that or on or off by default (respectively).
On 2015/11/10 15:24:25, blundell wrote:
> or on -> are on

Done.

https://codereview.chromium.org/1421003007/diff/1/components/browser_sync/bro...
components/browser_sync/browser/profile_sync_components_factory_impl.h:116:
const std::string history_disabled_pref_;
On 2015/11/10 15:24:25, blundell wrote:
> nit: So far we've been keeping this as a const char*. Seems like it's worth
> having consistency?

Done.

[Nicolas Zea, 2015-11-11 00:32:49]

Patchset #4 (id:60001) has been deleted

[blundell, 2015-11-12 10:03:35]

As an alternative solution to the problem of inverting the creation ordering
between ProfileSyncComponentsFactoryImpl and ChromeSyncClient, is it not
possible to just move the creation of ProfileSyncComponentsFactoryImpl inside
the constructor of ChromeSyncClient? That seems like it would be a simpler delta
as basically all of the current flow would still work as-is AFAICT. Then we
could also just pass the embedder callback into the
ProfileSyncComponentsFactoryImpl constructor rather than needing an explicit
client method to get it, which makes reasoning about lifetime issues more simple
as well (cf. my comment about whether WeakPtr is necessary).

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:465: weak_ptr_factory_.GetWeakPtr());
this shouldn't need to be a weak ptr given that this instance owns the factory,
right?

[blundell, 2015-11-12 10:33:47]

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:168:
OAuth2TokenService::ScopeSet scope_set;
it seems like this isn't used either?

[Nicolas Zea, 2015-11-12 18:50:50]

On 2015/11/12 10:03:35, blundell wrote:
> As an alternative solution to the problem of inverting the creation ordering
> between ProfileSyncComponentsFactoryImpl and ChromeSyncClient, is it not
> possible to just move the creation of ProfileSyncComponentsFactoryImpl inside
> the constructor of ChromeSyncClient? That seems like it would be a simpler
delta
> as basically all of the current flow would still work as-is AFAICT. Then we
> could also just pass the embedder callback into the
> ProfileSyncComponentsFactoryImpl constructor rather than needing an explicit
> client method to get it, which makes reasoning about lifetime issues more
simple
> as well (cf. my comment about whether WeakPtr is necessary).
> 

The problem I see with this approach is that it makes things difficult to test.
As it is, by passing the factory in, it allows tests to pass in either a mock
version of simply a version with overridden custom methods (which as you can see
from the size of this CL, plenty of tests do). Requiring tests to use the
FakeSyncClient basically means many tests will need to duplicate the logic of
the ChromeSyncClient, so I think I'd like to keep the factory injected.
Thoughts?

[Nicolas Zea, 2015-11-12 19:07:29]

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/chr...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/chr...
chrome/browser/sync/chrome_sync_client.cc:465: weak_ptr_factory_.GetWeakPtr());
On 2015/11/12 10:03:34, blundell wrote:
> this shouldn't need to be a weak ptr given that this instance owns the
factory,
> right?

Technically, but that depends on the caller doing the Right Thing when getting
this method. Given that's not code controlled by this class, I feel it's safer
(and not particularly expensive) to just vend a weak ptr.

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/1421003007/diff/80001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:168:
OAuth2TokenService::ScopeSet scope_set;
On 2015/11/12 10:33:47, blundell wrote:
> it seems like this isn't used either?

Good catch, done.

[blundell, 2015-11-12 19:57:00]

On 2015/11/12 18:50:50, Nicolas Zea wrote:
> On 2015/11/12 10:03:35, blundell wrote:
> > As an alternative solution to the problem of inverting the creation ordering
> > between ProfileSyncComponentsFactoryImpl and ChromeSyncClient, is it not
> > possible to just move the creation of ProfileSyncComponentsFactoryImpl
inside
> > the constructor of ChromeSyncClient? That seems like it would be a simpler
> delta
> > as basically all of the current flow would still work as-is AFAICT. Then we
> > could also just pass the embedder callback into the
> > ProfileSyncComponentsFactoryImpl constructor rather than needing an explicit
> > client method to get it, which makes reasoning about lifetime issues more
> simple
> > as well (cf. my comment about whether WeakPtr is necessary).
> > 
> 
> The problem I see with this approach is that it makes things difficult to
test.
> As it is, by passing the factory in, it allows tests to pass in either a mock
> version of simply a version with overridden custom methods (which as you can
see
> from the size of this CL, plenty of tests do). Requiring tests to use the
> FakeSyncClient basically means many tests will need to duplicate the logic of
> the ChromeSyncClient, so I think I'd like to keep the factory injected.
> Thoughts?

Ah, I missed that. What do you think about just adding a SetFactoryForTesting()
method to ChromeSyncClient that the tests in question can use? It seems like a
shame to have to make the already somewhat complex production flow around
creating and initializing the PSS, ProfileSyncComponentsFactoryImpl, and
<Whatever>SyncClient more complex for the sake of tests.

Re: the weak pointer, I agree with you that if it's a method on the SyncClient
interface then it makes sense for ChromeSyncClient to be defensive. But if the
callback is passed in as a constructor arg to ProfileSyncComponentsFactoryImpl
with a comment on that class's header that the passed-in callback needs to be
valid only for the lifetime of the ProfileSyncComponentsFactoryImpl instance
being created, I think it would be fine to switch to a raw pointer. The reason
I'm focusing on this isn't the performance cost of taking the weak pointer; it's
similar to the above, that it would be nice to keep the ownership and lifetime
relationships between these objects as simple and clear as possible.

[blundell, 2015-11-13 09:33:56]

As you're the owner here, it's your call as to which flow you prefer.

lgtm as-is with nits, or I'm happy to review again if you want to change to the
flow I suggested.

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_factory.cc:185:
sync_driver::SyncApiComponentFactory* sync_factory =
nit: This could be a scoped_ptr variable right?

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_mock.cc (right):

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_mock.cc:65:
sync_service->GetSyncClient()->Initialize(
nit: Any reason not to put this in the constructor of ProfileSyncServiceMock?

[Nicolas Zea, 2015-11-14 00:46:46]

Patchset #6 (id:120001) has been deleted

[Nicolas Zea, 2015-11-14 00:52:26]

I like your suggestion. We should focus on keeping the normal initialization as
simple as possible. Added the SetSyncApiComponentFactoryForTesting method, PTAL.

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_factory.cc:185:
sync_driver::SyncApiComponentFactory* sync_factory =
On 2015/11/13 09:33:56, blundell wrote:
> nit: This could be a scoped_ptr variable right?

This has been removed.

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_mock.cc (right):

https://codereview.chromium.org/1421003007/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_mock.cc:65:
sync_service->GetSyncClient()->Initialize(
On 2015/11/13 09:33:56, blundell wrote:
> nit: Any reason not to put this in the constructor of ProfileSyncServiceMock?

This has been removed.

[blundell, 2015-11-16 08:43:10]

LGTM

Test failures look real though, unfortunately.

https://codereview.chromium.org/1421003007/diff/140001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1421003007/diff/140001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:193: component_factory_.reset(new
ProfileSyncComponentsFactoryImpl(
The CL description says that the callback is being passed into the
ProfileSyncComponentsFactoryImpl at construction time but that doesn't match the
code :). One or the other should change.

[Nicolas Zea, 2015-11-16 19:13:31]

Description was changed from

==========
[Sync] Componentize ProfileSyncComponentsFactoryImpl

Componentize ProfileSyncComponentsFactoryImpl into browser_sync component.
As part of this, pull out RegisterDesktopDatatypes method into ChromeSyncClient
and pass it into the ProfileSyncComponentsFactoryImpl at construction time.
Similarly, a RegisterAndroidDataTypes method is passed in instead for Android
platforms.

Because profile_sync_components_factory_impl_unittest.cc was really duplicating
and testing the logic from ProfileSyncServiceFactory, went ahead and moved the
test cases over into profile_sync_service_factory_unittest.cc.

BUG=512768
==========

to

==========
[Sync] Componentize ProfileSyncComponentsFactoryImpl

Componentize ProfileSyncComponentsFactoryImpl into browser_sync component.
As part of this, pull out RegisterDesktopDatatypes method into ChromeSyncClient
and pass it into the ProfileSyncComponentsFactoryImpl at construction time.
Similarly, a RegisterAndroidDataTypes method is passed in instead for Android
platforms.

Because profile_sync_components_factory_impl_unittest.cc was really duplicating
and testing the logic from ProfileSyncServiceFactory, went ahead and moved the
test cases over into profile_sync_service_factory_unittest.cc.

BUG=512768
TBR=rdevlin.cronin@chromium.org
==========

[Nicolas Zea, 2015-11-16 19:13:31]

zea@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Nicolas Zea, 2015-11-16 19:28:14]

Description was changed from

==========
[Sync] Componentize ProfileSyncComponentsFactoryImpl

Componentize ProfileSyncComponentsFactoryImpl into browser_sync component.
As part of this, pull out RegisterDesktopDatatypes method into ChromeSyncClient
and pass it into the ProfileSyncComponentsFactoryImpl at construction time.
Similarly, a RegisterAndroidDataTypes method is passed in instead for Android
platforms.

Because profile_sync_components_factory_impl_unittest.cc was really duplicating
and testing the logic from ProfileSyncServiceFactory, went ahead and moved the
test cases over into profile_sync_service_factory_unittest.cc.

BUG=512768
TBR=rdevlin.cronin@chromium.org
==========

to

==========
[Sync] Componentize ProfileSyncComponentsFactoryImpl

Componentize ProfileSyncComponentsFactoryImpl into browser_sync component.
As part of this, pull out RegisterDesktopDatatypes method into ChromeSyncClient.
Similarly, a RegisterAndroidDataTypes is created instead for Android
platforms.

Because profile_sync_components_factory_impl_unittest.cc was really duplicating
and testing the logic from ProfileSyncServiceFactory, went ahead and moved the
test cases over into profile_sync_service_factory_unittest.cc.

BUG=512768
TBR=rdevlin.cronin@chromium.org
==========

[Nicolas Zea, 2015-11-16 20:28:00]

+TBR Devlin for callsite changes to the extension tests.

[Nicolas Zea, 2015-11-16 20:28:06]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-11-16 20:28:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org
Link to the patchset: https://codereview.chromium.org/1421003007/#ps180001
(title: "Fix sessions api test")

[commit-bot: I haz the power, 2015-11-16 20:29:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1421003007/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1421003007/180001

[commit-bot: I haz the power, 2015-11-16 21:08:55]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-11-16 21:09:50]

Patchset 8 (id:??) landed as
https://crrev.com/7db382299ca33ffb369f9c79a5331bbfbab259bc
Cr-Commit-Position: refs/heads/master@{#359916}

[loyso (OOO), 2015-11-17 03:23:34]

A revert of this CL (patchset #8 id:180001) has been created in
https://codereview.chromium.org/1451023005/ by loyso@chromium.org.

The reason for reverting is: Fails unit tests on Android builds:
ProfileSyncServiceFactoryTest.CreatePSSDisableOne
ProfileSyncServiceFactoryTest.CreatePSSDisableMultiple
ProfileSyncServiceFactoryTest.CreatePSSDefault.