[Sync] Split encryption state and logic out of PSS and SBHI.

[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

Potential followup CLs:

- Make SyncServiceCrypto an interface that SyncService exposes so the
  encryption related methods can be removed from SyncService.
- Provide SyncServiceCrypto a proxy object that talks directly to
  SyncEncryptionHandler instead of routing through the SyncEngine
  interface.
- Have SyncServiceCrypto implement DataTypeEncryptionHandler.

BUG=685337
TBR=pkasting

Review-Url: https://codereview.chromium.org/2663783002
Cr-Commit-Position: refs/heads/master@{#447470}
Committed: https://chromium.googlesource.com/chromium/src/+/f635e7f7a4d7e1f62bab36d230183e499627b9f7

[maxbogue, 2017-01-30 13:05:09]

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

[commit-bot: I haz the power, 2017-01-30 13:05:22]

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, 2017-01-30 14:14:08]

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

[commit-bot: I haz the power, 2017-01-30 14:14:09]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2017-01-30 18:49:00]

Description was changed from

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.

BUG=685337
==========

to

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

BUG=685337
==========

[maxbogue, 2017-01-30 19:12:42]

Description was changed from

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

BUG=685337
==========

to

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

Potential followup CLs:

- Make SyncServiceCrypto an interface that SyncService exposes so the
  encryption related methods can be removed from SyncService.
- Provide SyncServiceCrypto a proxy object that talks directly to
  SyncEncryptionHandler instead of routing through the SyncEngine
  interface.
- Have SyncServiceCrypto implement DataTypeEncryptionHandler.

BUG=685337
==========

[maxbogue, 2017-01-30 19:45:33]

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

[commit-bot: I haz the power, 2017-01-30 19:45:57]

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

[maxbogue, 2017-01-30 19:46:34]

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

[maxbogue, 2017-01-30 19:46:35]

Pavel and Sky, if you could both take a look I'd appreciate it! Sorry it's so
big, but I'm quite pleased with it overall.

[commit-bot: I haz the power, 2017-01-30 20:58:36]

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

[commit-bot: I haz the power, 2017-01-30 20:58:37]

Dry run: This issue passed the CQ dry run.

[skym, 2017-01-31 01:58:54]

lgtm

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (left):

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:506: // Note about setting
passphrases: There are different scenarios under which
We seemed to have lost this comment. Was that on purpose?

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:724: //
SyncServiceCrypto::BeginConfigureCatchUpBeforeClear() and the following two
These comments are odd. The whole catch up implies passphrase thing seems a bit
sketchy. I don't know how to make this better.

https://codereview.chromium.org/2663783002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_service_crypto.h (right):

https://codereview.chromium.org/2663783002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service_crypto.h:22: // This class functions as an
extension of SyncServiceBase that handles things
Extension is an odd word that I think you're trying to convey certain things,
but they are not conveyed, the reader doesn't know what you mean. It seems
you're explicitly trying to avoid having a dependency on the PSS/SSB, would it
be worth noting that?

https://codereview.chromium.org/2663783002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service_crypto.h:32: // See the SyncService header.
Usually we use interfaces when multiple classes implement the same exact
methods. Though I don't know if it'd simplify things here. Exposing
SyncServiceCrypto through SyncService would definitely solve this. It's kind of
nice that external to sync can always interact with us through SS, but at the
same time, does that really make sense if it grows to be monolithic?

[maxbogue, 2017-01-31 07:49:47]

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

[commit-bot: I haz the power, 2017-01-31 07:49:55]

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

[maxbogue, 2017-01-31 07:51:11]

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (left):

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:506: // Note about setting
passphrases: There are different scenarios under which
On 2017/01/31 01:58:54, skym wrote:
> We seemed to have lost this comment. Was that on purpose?

Yeah; it doesn't seem particularly useful to me. I can re-add it somewhere if
you'd like.

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
File components/browser_sync/profile_sync_service.h (right):

https://codereview.chromium.org/2663783002/diff/20001/components/browser_sync...
components/browser_sync/profile_sync_service.h:724: //
SyncServiceCrypto::BeginConfigureCatchUpBeforeClear() and the following two
On 2017/01/31 01:58:54, skym wrote:
> These comments are odd. The whole catch up implies passphrase thing seems a
bit
> sketchy. I don't know how to make this better.

Same :(

https://codereview.chromium.org/2663783002/diff/20001/components/sync/driver/...
File components/sync/driver/sync_service_crypto.h (right):

https://codereview.chromium.org/2663783002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service_crypto.h:22: // This class functions as an
extension of SyncServiceBase that handles things
On 2017/01/31 01:58:54, skym wrote:
> Extension is an odd word that I think you're trying to convey certain things,
> but they are not conveyed, the reader doesn't know what you mean. It seems
> you're explicitly trying to avoid having a dependency on the PSS/SSB, would it
> be worth noting that?

Good point. Changed the wording to be "a mostly independent component of
SyncServiceBase" (since it gets some of the methods injected). Hopefully that's
better...

https://codereview.chromium.org/2663783002/diff/20001/components/sync/driver/...
components/sync/driver/sync_service_crypto.h:32: // See the SyncService header.
On 2017/01/31 01:58:54, skym wrote:
> Usually we use interfaces when multiple classes implement the same exact
> methods. Though I don't know if it'd simplify things here. Exposing
> SyncServiceCrypto through SyncService would definitely solve this. It's kind
of
> nice that external to sync can always interact with us through SS, but at the
> same time, does that really make sense if it grows to be monolithic?

I think it would be good to do, but quite frankly I don't have time.
SyncServiceCrypto could just be an interface masking everything related to
crypto on the UI thread and SyncServiceCryptoImpl could go in driver_impl/ and
do the stuff. The only challenging part really would be fixing the tests that
rely on calling the SyncEncryptionHandler::Observer methods directly.

Sadly the SyncEncryptionHandler interface itself doesn't completely align, and
doesn't look like it would really make sense to.

[commit-bot: I haz the power, 2017-01-31 08:59:58]

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

[commit-bot: I haz the power, 2017-01-31 08:59:59]

Dry run: This issue passed the CQ dry run.

[pavely, 2017-02-01 01:42:04]

lgtm

[maxbogue, 2017-02-01 01:50:58]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2017-02-01 01:50:59]

The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org
Link to the patchset: https://codereview.chromium.org/2663783002/#ps40001
(title: "Tweak comment.")

[commit-bot: I haz the power, 2017-02-01 01:51:48]

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, 2017-02-01 02:00:55]

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

[commit-bot: I haz the power, 2017-02-01 02:00:56]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[maxbogue, 2017-02-01 07:38:12]

Description was changed from

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

Potential followup CLs:

- Make SyncServiceCrypto an interface that SyncService exposes so the
  encryption related methods can be removed from SyncService.
- Provide SyncServiceCrypto a proxy object that talks directly to
  SyncEncryptionHandler instead of routing through the SyncEngine
  interface.
- Have SyncServiceCrypto implement DataTypeEncryptionHandler.

BUG=685337
==========

to

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

Potential followup CLs:

- Make SyncServiceCrypto an interface that SyncService exposes so the
  encryption related methods can be removed from SyncService.
- Provide SyncServiceCrypto a proxy object that talks directly to
  SyncEncryptionHandler instead of routing through the SyncEngine
  interface.
- Have SyncServiceCrypto implement DataTypeEncryptionHandler.

BUG=685337
TBR=pkasting
==========

[maxbogue, 2017-02-01 07:39:25]

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

[maxbogue, 2017-02-01 07:39:25]

TBR pkasting for chrome/browser/autocomplete/

[maxbogue, 2017-02-01 07:39:32]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2017-02-01 07:39:42]

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, 2017-02-01 07:43:57]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485934772921670,
"parent_rev": "b802274a1bc5bce58c887bed7abee632c2a8f862", "commit_rev":
"f635e7f7a4d7e1f62bab36d230183e499627b9f7"}

[commit-bot: I haz the power, 2017-02-01 07:48:07]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485934772921670,
"parent_rev": "b802274a1bc5bce58c887bed7abee632c2a8f862", "commit_rev":
"f635e7f7a4d7e1f62bab36d230183e499627b9f7"}

[commit-bot: I haz the power, 2017-02-01 07:48:09]

Description was changed from

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

Potential followup CLs:

- Make SyncServiceCrypto an interface that SyncService exposes so the
  encryption related methods can be removed from SyncService.
- Provide SyncServiceCrypto a proxy object that talks directly to
  SyncEncryptionHandler instead of routing through the SyncEngine
  interface.
- Have SyncServiceCrypto implement DataTypeEncryptionHandler.

BUG=685337
TBR=pkasting
==========

to

==========
[Sync] Split encryption state and logic out of PSS and SBHI.

This change consolidates several pieces of state and logic regarding
encryption on the UI thread from ProfileSyncService and
SyncBackendHostImpl into a new class called SyncServiceCrypto.

This unblocks the SyncEngine work by removing several synchronous calls
from the SyncEngine interface, and removing logic from SBHI.

The SyncEncryptionHandler::Observer interface is now used to proxy
calls from the sync thread to the crypto object on the UI thread. SBHC
therefore no longer needs to implement it, SBHI doesn't need weird other
versions of the methods, and SyncEngineHost doesn't need duplicate
versions of several of the methods.

Other notes:

- Added IsNigoriEnabled to DataTypeManager because it no longer made
  sense on SyncEngine and it seemed to fit there.
- Moved ClearServerDataEvents to the SyncService header because I needed
  it in the new object.
- NotifyObservers and friends have been moved to SyncServiceBase so they
  can be bound to the crypto object.
- ProfileSyncService::GetEncryptionObserverForTest() was added to
  support the surprising amount of tests abusing the fact that those
  methods were technically public before.
- The HasUnrecoverableError() check has been removed from
  OnPassphraseRequired() because it doesn't seem necessary.

This change should hopefully have no functional changes.

Potential followup CLs:

- Make SyncServiceCrypto an interface that SyncService exposes so the
  encryption related methods can be removed from SyncService.
- Provide SyncServiceCrypto a proxy object that talks directly to
  SyncEncryptionHandler instead of routing through the SyncEngine
  interface.
- Have SyncServiceCrypto implement DataTypeEncryptionHandler.

BUG=685337
TBR=pkasting

Review-Url: https://codereview.chromium.org/2663783002
Cr-Commit-Position: refs/heads/master@{#447470}
Committed:
https://chromium.googlesource.com/chromium/src/+/f635e7f7a4d7e1f62bab36d23018...
==========

[commit-bot: I haz the power, 2017-02-01 07:48:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f635e7f7a4d7e1f62bab36d23018...

[Peter Kasting, 2017-02-01 09:12:23]

LGTM