[Sync] SyncEngine refactor part 2: SyncServiceBase.

[Sync] SyncEngine refactor part 2: SyncServiceBase.

This change introduces SyncServiceBase as a base class of PSS and
consolidates engine initialization logic into it from PSS and SBHI.

This new class will enable us to siphon logic out of SBHI until it can
become SyncEngineProxy (no business logic). This change also begins
aligning SBHC with the SyncEngine interface, so it can eventually become
SyncEngineImpl.

BUG=669967
Committed: https://crrev.com/fa5a4db658dda186eedfd7b5989f9b2abbe48c8f
Cr-Commit-Position: refs/heads/master@{#438620}

[maxbogue, 2016-12-08 17:18:25]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 17:18:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 18:05:10]

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

[commit-bot: I haz the power, 2016-12-08 18:05:11]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[maxbogue, 2016-12-08 20:20:53]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 20:21:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 21:03:58]

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

[commit-bot: I haz the power, 2016-12-08 21:03:59]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[maxbogue, 2016-12-10 00:01:21]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 00:02:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 00:06:22]

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

[commit-bot: I haz the power, 2016-12-10 00:06:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[maxbogue, 2016-12-10 00:55:44]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 00:56:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 02:10:51]

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

[commit-bot: I haz the power, 2016-12-10 02:10:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[maxbogue, 2016-12-12 18:16:48]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 18:17:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-12 19:31:37]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:33:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-12 20:55:51]

maxbogue@chromium.org changed reviewers:
+ pavely@chromium.org, skym@chromium.org

[maxbogue, 2016-12-12 20:55:52]

Sky, Pavel, PTAL!

[commit-bot: I haz the power, 2016-12-12 22:26:18]

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

[commit-bot: I haz the power, 2016-12-12 22:26:19]

Dry run: This issue passed the CQ dry run.

[pavely, 2016-12-13 22:50:15]

lgtm

[skym, 2016-12-14 19:02:35]

https://codereview.chromium.org/2559123002/diff/100001/components/browser_syn...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2559123002/diff/100001/components/browser_syn...
components/browser_sync/profile_sync_service.cc:599: if
(!IsFirstSetupComplete())
What's going on here? Is this copied from somewhere? Why don't you do this for
first setup?

https://codereview.chromium.org/2559123002/diff/100001/components/sync/driver...
File components/sync/driver/sync_service_base.h (right):

https://codereview.chromium.org/2559123002/diff/100001/components/sync/driver...
components/sync/driver/sync_service_base.h:29: // common functionality and
member variables.
Common functionality? Between who? Me, myself, and PSS? I would like this to
explain the local separation of SyncService functionality that should go into
SSB, and functionality that is outside of the SSB's scope.

Also, why does it exist? Why not just have all of this in PSS?

[maxbogue, 2016-12-14 19:36:53]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 19:37:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2016-12-14 19:38:10]

https://codereview.chromium.org/2559123002/diff/100001/components/browser_syn...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2559123002/diff/100001/components/browser_syn...
components/browser_sync/profile_sync_service.cc:599: if
(!IsFirstSetupComplete())
On 2016/12/14 19:02:35, skym wrote:
> What's going on here? Is this copied from somewhere? Why don't you do this for
> first setup?

It was moved from inside InitializeEngine (line 541). It's only on first start
of sync; I added a comment clarifying a little.

https://codereview.chromium.org/2559123002/diff/100001/components/sync/driver...
File components/sync/driver/sync_service_base.h (right):

https://codereview.chromium.org/2559123002/diff/100001/components/sync/driver...
components/sync/driver/sync_service_base.h:29: // common functionality and
member variables.
On 2016/12/14 19:02:35, skym wrote:
> Common functionality? Between who? Me, myself, and PSS? I would like this to
> explain the local separation of SyncService functionality that should go into
> SSB, and functionality that is outside of the SSB's scope.
> 
> Also, why does it exist? Why not just have all of this in PSS?

I updated the comment. Anything that can be inside the sync component should be
in SSB and not PSS. We want to make PSS smaller.

[skym, 2016-12-14 19:52:47]

lgtm

[maxbogue, 2016-12-14 20:03:35]

The CQ bit was unchecked by maxbogue@chromium.org

[maxbogue, 2016-12-14 20:03:39]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2016-12-14 20:03:40]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/2559123002/#ps120001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-12-14 20:04:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 21:06:19]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481745819736270,
"parent_rev": "b1fe83ffcd68bbef7bbaca0eecfa3fad38a5df21", "commit_rev":
"11c2d8fc1f6f5c8ebfa7d0d0f022f375de79f33a"}

[commit-bot: I haz the power, 2016-12-14 21:06:47]

Description was changed from

==========
[Sync] SyncEngine refactor part 2: SyncServiceBase.

This change introduces SyncServiceBase as a base class of PSS and
consolidates engine initialization logic into it from PSS and SBHI.

This new class will enable us to siphon logic out of SBHI until it can
become SyncEngineProxy (no business logic). This change also begins
aligning SBHC with the SyncEngine interface, so it can eventually become
SyncEngineImpl.

BUG=669967
==========

to

==========
[Sync] SyncEngine refactor part 2: SyncServiceBase.

This change introduces SyncServiceBase as a base class of PSS and
consolidates engine initialization logic into it from PSS and SBHI.

This new class will enable us to siphon logic out of SBHI until it can
become SyncEngineProxy (no business logic). This change also begins
aligning SBHC with the SyncEngine interface, so it can eventually become
SyncEngineImpl.

BUG=669967

Review-Url: https://codereview.chromium.org/2559123002
==========

[commit-bot: I haz the power, 2016-12-14 21:06:48]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-14 21:08:46]

Description was changed from

==========
[Sync] SyncEngine refactor part 2: SyncServiceBase.

This change introduces SyncServiceBase as a base class of PSS and
consolidates engine initialization logic into it from PSS and SBHI.

This new class will enable us to siphon logic out of SBHI until it can
become SyncEngineProxy (no business logic). This change also begins
aligning SBHC with the SyncEngine interface, so it can eventually become
SyncEngineImpl.

BUG=669967

Review-Url: https://codereview.chromium.org/2559123002
==========

to

==========
[Sync] SyncEngine refactor part 2: SyncServiceBase.

This change introduces SyncServiceBase as a base class of PSS and
consolidates engine initialization logic into it from PSS and SBHI.

This new class will enable us to siphon logic out of SBHI until it can
become SyncEngineProxy (no business logic). This change also begins
aligning SBHC with the SyncEngine interface, so it can eventually become
SyncEngineImpl.

BUG=669967

Committed: https://crrev.com/fa5a4db658dda186eedfd7b5989f9b2abbe48c8f
Cr-Commit-Position: refs/heads/master@{#438620}
==========

[commit-bot: I haz the power, 2016-12-14 21:08:48]

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