[Sync] Componentize ProfileSyncService

[Sync] Componentize ProfileSyncService

This CL componentizes ProfileSyncService into //components/browser_sync/browser
for clean integration on iOS. There are no problematic dependencies, so the
componentization is straightforward.

BUG=512825
TBR=rogerta,ellyjones,jochen

Committed: https://crrev.com/7282b51bd5ceb6b6abbe3b9ae48e9a748bfbbd9f
Cr-Commit-Position: refs/heads/master@{#358567}

[blundell, 2015-11-05 13:04:44]

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

[blundell, 2015-11-05 13:04:45]

\o/

[vabr (Chromium), 2015-11-05 13:11:59]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-11-05 13:12:00]

This is great!

Every file with *passwords* in the name LGTM.

Cheers,
Vaclav

[maxbogue, 2015-11-05 19:01:59]

maxbogue@chromium.org changed reviewers:
+ maxbogue@chromium.org

[maxbogue, 2015-11-05 19:01:59]

This was probably decided in some doc/email thread I didn't see, but why the
/browser? Is there ever gonna be anything in browser_sync that doesn't live in
browser_sync/browser?

[blundell, 2015-11-05 19:13:35]

On 2015/11/05 19:01:59, maxbogue wrote:
> This was probably decided in some doc/email thread I didn't see, but why the
> /browser? Is there ever gonna be anything in browser_sync that doesn't live in
> browser_sync/browser?

There already is on trunk. browser_sync_switches.* has to live in
//components/browser_sync/common because //cloud_print depends on it and that
code doesn't live in the browser process. I agree that it's :P.

[blundell, 2015-11-05 19:13:53]

On 2015/11/05 19:13:35, blundell wrote:
> On 2015/11/05 19:01:59, maxbogue wrote:
> > This was probably decided in some doc/email thread I didn't see, but why the
> > /browser? Is there ever gonna be anything in browser_sync that doesn't live
in
> > browser_sync/browser?
> 
> There already is on trunk. browser_sync_switches.* has to live in
> //components/browser_sync/common because //cloud_print depends on it and that
> code doesn't live in the browser process. I agree that it's :P.

[blundell, 2015-11-05 19:14:15]

On 2015/11/05 19:13:53, blundell wrote:
> On 2015/11/05 19:13:35, blundell wrote:
> > On 2015/11/05 19:01:59, maxbogue wrote:
> > > This was probably decided in some doc/email thread I didn't see, but why
the
> > > /browser? Is there ever gonna be anything in browser_sync that doesn't
live
> in
> > > browser_sync/browser?
> > 
> > There already is on trunk. browser_sync_switches.* has to live in
> > //components/browser_sync/common because //cloud_print depends on it and
that
> > code doesn't live in the browser process. I agree that it's :P.

(on trunk as of earlier today ;)

[maxbogue, 2015-11-05 20:40:21]

On 2015/11/05 19:14:15, blundell wrote:
> On 2015/11/05 19:13:53, blundell wrote:
> > On 2015/11/05 19:13:35, blundell wrote:
> > > On 2015/11/05 19:01:59, maxbogue wrote:
> > > > This was probably decided in some doc/email thread I didn't see, but why
> the
> > > > /browser? Is there ever gonna be anything in browser_sync that doesn't
> live
> > in
> > > > browser_sync/browser?
> > > 
> > > There already is on trunk. browser_sync_switches.* has to live in
> > > //components/browser_sync/common because //cloud_print depends on it and
> that
> > > code doesn't live in the browser process. I agree that it's :P.
> 
> (on trunk as of earlier today ;)

I would have rather those lived in
//components/sync_driver/sync_driver_switches.h and not have a /common folder...
it makes me sad to have an extra layer of folders just for that.

[maxbogue, 2015-11-05 20:40:39]

https://codereview.chromium.org/1419103009/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/1419103009/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:21: #include
<components/password_manager/core/browser/password_ui_utils.h>
Looks like this should just be quotes instead of being moved.

[blundell, 2015-11-05 20:43:43]

On 2015/11/05 20:40:21, maxbogue wrote:
> On 2015/11/05 19:14:15, blundell wrote:
> > On 2015/11/05 19:13:53, blundell wrote:
> > > On 2015/11/05 19:13:35, blundell wrote:
> > > > On 2015/11/05 19:01:59, maxbogue wrote:
> > > > > This was probably decided in some doc/email thread I didn't see, but
why
> > the
> > > > > /browser? Is there ever gonna be anything in browser_sync that doesn't
> > live
> > > in
> > > > > browser_sync/browser?
> > > > 
> > > > There already is on trunk. browser_sync_switches.* has to live in
> > > > //components/browser_sync/common because //cloud_print depends on it and
> > that
> > > > code doesn't live in the browser process. I agree that it's :P.
> > 
> > (on trunk as of earlier today ;)
> 
> I would have rather those lived in
> //components/sync_driver/sync_driver_switches.h and not have a /common
folder...
> it makes me sad to have an extra layer of folders just for that.

It would have been the same problem at the //components/sync_driver level...all
of that code is currently browser-specific code. Fundamentally, there's a reason
that chrome_switches.h is in //chrome/common. If a feature has switches and
they're actually used by both the browser process code and other processes'
code, then when those switches are componentized they can't live in the same
target as the browser process-specific code for that feature.

[Nicolas Zea, 2015-11-06 00:02:48]

Awesome!!

*sync* LGTM, with the request that sync_type_preference_provider actually go
into sync_driver instead (it's closer to SyncPrefs than anything else, and there
are some TODOs to make the SyncPrefs implement the SyncTypePreferenceProvider
interface).

[blundell, 2015-11-06 10:10:58]

Thanks! Moved the file.

https://codereview.chromium.org/1419103009/diff/1/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/password_manager_handler.cc (right):

https://codereview.chromium.org/1419103009/diff/1/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/password_manager_handler.cc:21: #include
<components/password_manager/core/browser/password_ui_utils.h>
On 2015/11/05 20:40:39, maxbogue wrote:
> Looks like this should just be quotes instead of being moved.

Nice catch. Done. (Also moved the conditional include just above to below the
unconditional includes).

[blundell, 2015-11-06 10:13:06]

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

[blundell, 2015-11-06 10:13:07]

+avi@ for the DEPS entry of //components/browser_sync/browser on //ui/base/l10n

[Avi (use Gerrit), 2015-11-06 20:32:05]

On 2015/11/06 10:13:07, blundell wrote:
> +avi@ for the DEPS entry of //components/browser_sync/browser on
//ui/base/l10n

LGTM for that.

[maxbogue, 2015-11-06 23:23:41]

lgtm

http://crrev.com/1407373006 has landed so you should be good to move forward
with this!

[blundell, 2015-11-09 07:10:38]

Description was changed from

==========
[Sync] Componentize ProfileSyncService

This CL componentizes ProfileSyncService into //components/browser_sync/browser
for clean integration on iOS. There are no problematic dependencies, so the
componentization is straightforward.

BUG=512825
==========

to

==========
[Sync] Componentize ProfileSyncService

This CL componentizes ProfileSyncService into //components/browser_sync/browser
for clean integration on iOS. There are no problematic dependencies, so the
componentization is straightforward.

BUG=512825
TBR=rogerta,ellyjones,jochen
==========

[blundell, 2015-11-09 07:10:39]

blundell@chromium.org changed reviewers:
+ ellyjones@chromium.org, jochen@chromium.org, rogerta@chromium.org

[blundell, 2015-11-09 07:12:28]

TBR=jochen for //chrome include path changes outside of //chrome/browser/*sync*
TBR=rogerta for dependency from //components/browser_sync/browser to
//google_apis (this is conceptually the same dependency as the already-existing
one from //components/sync_driver)
TBR=ellyjones for dependency from //components/browser_sync/browser to //net
(this is conceptually the same dependency as the already-existing one from
//components/sync_driver)

[blundell, 2015-11-09 07:12:58]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-11-09 07:12:59]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
zea@chromium.org, maxbogue@chromium.org, avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1419103009/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-09 07:13:29]

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

[commit-bot: I haz the power, 2015-11-09 07:21:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-09 07:22:23]

Patchset 3 (id:??) landed as
https://crrev.com/7282b51bd5ceb6b6abbe3b9ae48e9a748bfbbd9f
Cr-Commit-Position: refs/heads/master@{#358567}

[Elly Fong-Jones, 2015-11-09 13:11:14]

lgtm