[Sync] Introduce GmsCoreSyncListener.

[Sync] Introduce GmsCoreSyncListener.

This change introduces a framework for sharing the user's custom
passphrase encryption key with GmsCore. This is to prevent users from
seeing the custom passphrase dialog pop up in GmsCore after they have
already entered it in Chrome.

BUG=552106
Committed: https://crrev.com/774f31729ab739d01f63aa3f10b18140b8ea1df2
Cr-Commit-Position: refs/heads/master@{#360344}

[maxbogue, 2015-11-13 22:26:02]

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

[maxbogue, 2015-11-13 22:26:03]

Hey Tommy, I finally got around to fixing the last test issues with this. PTAL!

[nyquist, 2015-11-17 20:33:08]

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

[nyquist, 2015-11-17 20:33:09]

lgtm, but it'd be great if zea@ could also look at the crypto-things.

zea: could you look at crypto things?

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/GmsCoreSyncListener.java
(right):

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/GmsCoreSyncListener.java:28:
ProfileSyncService syncService = ProfileSyncService.get();
Nit: ThreadUtils.assertOnUiThread()?

[maxbogue, 2015-11-17 22:14:12]

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/GmsCoreSyncListener.java
(right):

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/sync/GmsCoreSyncListener.java:28:
ProfileSyncService syncService = ProfileSyncService.get();
On 2015/11/17 20:33:09, nyquist wrote:
> Nit: ThreadUtils.assertOnUiThread()?

ProfileSyncService.get() already does that, and this method is only ever called
on the UI thread by PSS... would you still like me to add it?

[Nicolas Zea, 2015-11-17 22:47:45]

LGTM with some nits

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/GmsCoreSyncListenerTest.java
(right):

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/GmsCoreSyncListenerTest.java:62:
public void testGetsKey() throws Throwable {
nit: is it normal to have the tests defined above the helper methods they use?
Maybe move these two down to the end of the class?

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:1867:
sync_driver::SystemEncryptor encryptor;
DCHECK that GetEncryptionBootstrapToken() isn't empty?

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:279: std::string
GetCustomPassphraseKey() const;
This section is for SyncService implememtations. Move below and add a comment
describing what it does?

[maxbogue, 2015-11-17 23:25:31]

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/sync_she...
File
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/GmsCoreSyncListenerTest.java
(right):

https://codereview.chromium.org/1414203016/diff/60001/chrome/android/sync_she...
chrome/android/sync_shell/javatests/src/org/chromium/chrome/browser/sync/GmsCoreSyncListenerTest.java:62:
public void testGetsKey() throws Throwable {
On 2015/11/17 22:47:45, Nicolas Zea wrote:
> nit: is it normal to have the tests defined above the helper methods they use?
> Maybe move these two down to the end of the class?

Yeah, it's pretty common to have all the helpers down at the bottom so it's
easier to find the actual tests and see what they do. Sometimes the helper
section is significantly longer than the tests themselves, and most helpers are
fairly self-explanatory by name, so it seems to make sense to do it this way.

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.cc (right):

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.cc:1867:
sync_driver::SystemEncryptor encryptor;
On 2015/11/17 22:47:45, Nicolas Zea wrote:
> DCHECK that GetEncryptionBootstrapToken() isn't empty?

It actually is empty here sometimes. That's why GmsCoreSyncListener doesn't do
anything if the key is empty. I couldn't find a condition to check that would
guarantee this was set. I could add a HasCustomPassphraseKey(), but I was trying
to add as few methods as possible to keep the PSS bridge less cluttered. Would
you prefer that?

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
File components/browser_sync/browser/profile_sync_service.h (right):

https://codereview.chromium.org/1414203016/diff/60001/components/browser_sync...
components/browser_sync/browser/profile_sync_service.h:279: std::string
GetCustomPassphraseKey() const;
On 2015/11/17 22:47:45, Nicolas Zea wrote:
> This section is for SyncService implememtations. Move below and add a comment
> describing what it does?

Whoops, good catch. Meant to move this when I first got stuff working but it
slipped through the cracks.

[Nicolas Zea, 2015-11-18 00:37:28]

Hmm, I see. As long as testGetsKey is making sure that we don't regress and stop
notifying syncStateChanged after updating an encryption key, LGTM

[maxbogue, 2015-11-18 16:19:27]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-11-18 16:19:27]

The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1414203016/#ps80001
(title: "Move GetCustomPassphraseKey declaration.")

[commit-bot: I haz the power, 2015-11-18 16:19:45]

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

[commit-bot: I haz the power, 2015-11-18 16:27:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-18 16:28:20]

Patchset 5 (id:??) landed as
https://crrev.com/774f31729ab739d01f63aa3f10b18140b8ea1df2
Cr-Commit-Position: refs/heads/master@{#360344}