[sync] Upstream the Android ProfileSyncService

[sync] Upstream the Android ProfileSyncService

This creates a Java-layer ProfileSyncService that is the only class that
should be used from downstream Chrome for Android. The native
counterparts should not be used.

In the process, a hashing utility and also, some identity code and their
respective tests had to be upstreamed. No functional changes have been
made to them.

BUG=159203
TBR=sky@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186798

[nyquist, 2013-02-22 21:42:09]

yfriedman: chrome/android/java[tests], chrome/browser/android, sync/android/java
atwilson/tim: chrome/browser/sync, sync/android/java

[Yaron, 2013-02-23 01:38:49]

didn't get through all of pssa.cc but here's initial comments.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
File chrome/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:43: <!-- Sync error string for
generic error. [CHAR-LIMIT=80] -->
Please re-use native strings wherever possible.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/GoogleServiceAuthError.java
(right):

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/GoogleServiceAuthError.java:11:
* google_apis/gaia/google_service_auth_error.h.
Might be worth auto-generating this in a future CL. See
page_transition_types_list.h for an example.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:59:
// Native ProfileSyncServiceAndroid object. Can not be final since we set it to
0 in destroy().
Since destroy isn't called let's just delete that function and make this final

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:87:
// been set up, but ProfileSyncService::Startup() wont be called until
s/wont/won't/

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:138:
assert mNativeProfileSyncServiceAndroid != 0;
I believe these asserts are spurious. It's set in the ctor. Please remove
throughout

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:152:
* Must be run on the main thread since it is contacting the sync engine directly
Comment is redundant (it's assumed everywhere)

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:187:
Log.d(TAG, "Handling request for auth token from sync engine");
Log Necessary?

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:216:
if (mNativeProfileSyncServiceAndroid != 0) {
If destroy can go away, i think this check can too. I'm guess this was all old
code and we used to not init native in ctor?

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:262:
* Returns true if sync has been migrated.
This is helpful..

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:447:
Log.d(TAG, "Setting sync setup_in_progress = " + inProgress);
remove

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:452:
mListeners.add(listener);
Assert UIThread so it's clear that callback will come back on same thread (as
came up with my TemplateUrlService patch). Next function too

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:472:
new ArrayList<SyncStateChangedListener>(mListeners);
Hmm. What if mListeners is a CopyOnWriteArrayList? I guess it depends on how
often syncStateChnaged is fired. Looks like not often so maybe fine as is. (Just
want to raise for consideration)

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:503:

Are whitespaces helpful here?

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:92: VLOG(2) << "Trying to
update token for user " << signed_in_username;
DVLOG?

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:100: LOG(ERROR) << "Sync
service not found.";
Redundant error: remove

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:601:
g_profile_sync_service_android = new ProfileSyncServiceAndroid(env, obj);
This is asserted in the java-layer. Remove global. We can also technically have
multiple without issue so it seems unnecessary here.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:18: // Android wrapper of the
ProfileSyncService which provides access from the Java
Nit: ws above

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:210: bool
RegisterProfileSyncServiceAndroid(JNIEnv* env);
Nit: make static method of class

[Andrew T Wilson (Slow), 2013-02-25 16:23:35]

Overall LG, but some nits.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:59: LOG(ERROR) << "Browser
process or profile manager not initialized";
I suspect all these LOG(ERROR) these should be NOTREACHED(), not LOG(). At
least, they should be DLOG() so they don't show up in release versions?

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:75: if (sync_service_ ==
NULL) {
These checks are odd. I think they should be DCHECKs(), or even better, just add
one DCHECK(sync_service_) in the constructor.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:191: VLOG(2) << "Ignoring
call to EnableSync() because sync is already enabled";
Change all VLOGs to DVLOG()

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:218:
profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, name);
Should change our uses of kGoogleServicesUsername to
SigninManager::Get/SetAuthenticatedUsername()

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:252: PrefService*
pref_service = profile_->GetPrefs();
We should use SyncPrefs for accessing any sync preferences.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:27: void NudgeSyncer(JNIEnv*
env,
Is this only called from tests? We should rename this NudgeSyncerForTest().

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:43: // |auth_token| is empty,
a new |auth_token| is requested from the UI thread
Sigh, I never did get around to refactoring this to either require a non-empty
auth_token, or get rid of auth_token entirely (always request it from the UI
thread if needed).

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:185: virtual void
OnStateChanged() OVERRIDE;
blank line before private:

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:187: virtual
~ProfileSyncServiceAndroid();
I forget how this works now - who owns/frees this object?

[nyquist, 2013-02-27 05:19:16]

Addressed comments. PTAL

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
File chrome/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:43: <!-- Sync error string for
generic error. [CHAR-LIMIT=80] -->
On 2013/02/23 01:38:49, Yaron wrote:
> Please re-use native strings wherever possible.

I have commented below on the different native strings available in native
today. Some of them are much longer.

Do you want me to use some from native, and some from strings.xml? Or should I
move all these Java/Android-strings to generated_resources.grd?

Otherwise I vote for keeping as is until strings.xml is auto-generated instead.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:44: <string
name="sync_error_generic">Sync error</string>
IDS_SYNC_ERROR_BUBBLE_VIEW_TITLE =
Sync Error

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:46: <string
name="sync_error_ga">Please update your sign in details.</string>
IDS_SYNC_SIGN_IN_ERROR_BUBBLE_VIEW_MESSAGE =
<ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> could not sync your data
because your account sign in details are out of date.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:48: <string
name="sync_error_passphrase">Please update your sync passphrase.</string>
Perfect match
IDS_SYNC_PASSWORD_SYNC_ATTENTION = IDS_SYNC_CONFIGURE_ENCRYPTION =
Please update your sync passphrase.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:50: <string
name="sync_error_connection">Could not connect to the sync server.</string>
IDS_SYNC_SERVER_IS_UNREACHABLE =
<ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> could not sync your data
because it could not connect to the sync server. Retrying...

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:52: <string
name="sync_error_domain">Sync is not available for your domain.</string>
IDS_SYNC_UNAVAILABLE_ERROR_BUBBLE_VIEW_MESSAGE =
<ph name="PRODUCT_NAME">$1<ex>Google Chrome</ex></ph> could not sync your data
because Sync is not available for your domain.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:54: <string
name="sync_error_password_incorrect">The password you entered is
incorrect.</string>
IDS_INCORRECT_PASSPHRASE_WARNING =
The passphrase you entered is incorrect.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:56: <string
name="sync_error_asp_error">Enter your Google Account password first, not an
application specific password.</string>
Text matches, but native adds a breakline.
IDS_SYNC_ASP_PASSWORD_WARNING_TEXT =
Enter your Google Account password first,
<ph name="LINE_BREAK">&lt;br /&gt;<ex>&lt;br /&gt;</ex></ph>
not an application specific password.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/res/value...
chrome/android/java/res/values/strings.xml:58: <string
name="sync_error_service_unavailable">The service is not available; try again
later.</string>
IDS_SYNC_SETUP_ABORTED_BY_PENDING_CLEAR =
The sync server is busy, please try again later.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/GoogleServiceAuthError.java
(right):

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/GoogleServiceAuthError.java:11:
* google_apis/gaia/google_service_auth_error.h.
On 2013/02/23 01:38:49, Yaron wrote:
> Might be worth auto-generating this in a future CL. See
> page_transition_types_list.h for an example.

It seems like that template does not map to Java strings (nor native strings).
Also, it does not use an inner class for the enum value.

OK to punt for later?

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:59:
// Native ProfileSyncServiceAndroid object. Can not be final since we set it to
0 in destroy().
On 2013/02/23 01:38:49, Yaron wrote:
> Since destroy isn't called let's just delete that function and make this final

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:87:
// been set up, but ProfileSyncService::Startup() wont be called until
On 2013/02/23 01:38:49, Yaron wrote:
> s/wont/won't/

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:138:
assert mNativeProfileSyncServiceAndroid != 0;
On 2013/02/23 01:38:49, Yaron wrote:
> I believe these asserts are spurious. It's set in the ctor. Please remove
> throughout

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:152:
* Must be run on the main thread since it is contacting the sync engine directly
On 2013/02/23 01:38:49, Yaron wrote:
> Comment is redundant (it's assumed everywhere)

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:187:
Log.d(TAG, "Handling request for auth token from sync engine");
On 2013/02/23 01:38:49, Yaron wrote:
> Log Necessary?

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:216:
if (mNativeProfileSyncServiceAndroid != 0) {
On 2013/02/23 01:38:49, Yaron wrote:
> If destroy can go away, i think this check can too. I'm guess this was all old
> code and we used to not init native in ctor?

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:262:
* Returns true if sync has been migrated.
On 2013/02/23 01:38:49, Yaron wrote:
> This is helpful..

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:447:
Log.d(TAG, "Setting sync setup_in_progress = " + inProgress);
On 2013/02/23 01:38:49, Yaron wrote:
> remove

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:452:
mListeners.add(listener);
On 2013/02/23 01:38:49, Yaron wrote:
> Assert UIThread so it's clear that callback will come back on same thread (as
> came up with my TemplateUrlService patch). Next function too

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/android/java/src/org/c...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:503:

On 2013/02/23 01:38:49, Yaron wrote:
> Are whitespaces helpful here?

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:59: LOG(ERROR) << "Browser
process or profile manager not initialized";
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> I suspect all these LOG(ERROR) these should be NOTREACHED(), not LOG(). At
> least, they should be DLOG() so they don't show up in release versions?

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:75: if (sync_service_ ==
NULL) {
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> These checks are odd. I think they should be DCHECKs(), or even better, just
add
> one DCHECK(sync_service_) in the constructor.

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:92: VLOG(2) << "Trying to
update token for user " << signed_in_username;
On 2013/02/23 01:38:49, Yaron wrote:
> DVLOG?

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:100: LOG(ERROR) << "Sync
service not found.";
On 2013/02/23 01:38:49, Yaron wrote:
> Redundant error: remove

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:191: VLOG(2) << "Ignoring
call to EnableSync() because sync is already enabled";
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> Change all VLOGs to DVLOG()

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:218:
profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, name);
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> Should change our uses of kGoogleServicesUsername to
> SigninManager::Get/SetAuthenticatedUsername()

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:252: PrefService*
pref_service = profile_->GetPrefs();
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> We should use SyncPrefs for accessing any sync preferences.

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:601:
g_profile_sync_service_android = new ProfileSyncServiceAndroid(env, obj);
On 2013/02/23 01:38:49, Yaron wrote:
> This is asserted in the java-layer. Remove global. We can also technically
have
> multiple without issue so it seems unnecessary here.

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:18: // Android wrapper of the
ProfileSyncService which provides access from the Java
On 2013/02/23 01:38:49, Yaron wrote:
> Nit: ws above

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:27: void NudgeSyncer(JNIEnv*
env,
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> Is this only called from tests? We should rename this NudgeSyncerForTest().

No, this is in fact how we nudge the syncer from Java (for example through
remote invalidations).

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:43: // |auth_token| is empty,
a new |auth_token| is requested from the UI thread
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> Sigh, I never did get around to refactoring this to either require a non-empty
> auth_token, or get rid of auth_token entirely (always request it from the UI
> thread if needed).

I could file a bug about it if you want? Or do you want me to do that in this
CL?

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:185: virtual void
OnStateChanged() OVERRIDE;
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> blank line before private:

Done.

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:187: virtual
~ProfileSyncServiceAndroid();
On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> I forget how this works now - who owns/frees this object?

It is never destroyed...

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:210: bool
RegisterProfileSyncServiceAndroid(JNIEnv* env);
On 2013/02/23 01:38:49, Yaron wrote:
> Nit: make static method of class

Done.

[Andrew T Wilson (Slow), 2013-02-27 12:02:35]

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.h:43: // |auth_token| is empty,
a new |auth_token| is requested from the UI thread
I don't think that it's a pressing issue. Feel free to log a bug if you like but
don't bother addressing in this CL.

[Yaron, 2013-02-27 23:10:25]

lgtm

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:72: if
(!sync_service_->HasObserver(this)) {
only called in ctor, remove guard. Also, the other stuff is unrelated to adding
observer so maybe just inline in constructor.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:105: // Must be called on
the UI thread.
remove comment (asserted on first line)

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:380: static jstring
GetSyncEnterCustomPassphraseBodyText(JNIEnv* env, jclass clazz) {
Nit: convert to member function (this becomes a scoped return) and get rid of
the release.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:506: void
ProfileSyncServiceAndroid::InvalidateAuthToken() {
In theory this could be moved to TokenServiceAndroid but seems like overkill at
this point. Also, this is called from here in native so it's not like we need to
expose a java hook.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:530: jobject obj,
nit: indentation off

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.h:26: void Destroy(JNIEnv*env,
jobject obj);
nit: insert space. also, this is uncalled and could potentially just be dropped.

[nyquist, 2013-02-28 22:38:50]

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:72: if
(!sync_service_->HasObserver(this)) {
On 2013/02/27 23:10:25, Yaron wrote:
> only called in ctor, remove guard. Also, the other stuff is unrelated to
adding
> observer so maybe just inline in constructor.

Done.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:105: // Must be called on
the UI thread.
On 2013/02/27 23:10:25, Yaron wrote:
> remove comment (asserted on first line)

Done.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:506: void
ProfileSyncServiceAndroid::InvalidateAuthToken() {
On 2013/02/27 23:10:25, Yaron wrote:
> In theory this could be moved to TokenServiceAndroid but seems like overkill
at
> this point. Also, this is called from here in native so it's not like we need
to
> expose a java hook.

Yes, we should move auth token and possibly part of signing in/out things out of
this going forward.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:530: jobject obj,
On 2013/02/27 23:10:25, Yaron wrote:
> nit: indentation off

Done.

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/1018/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.h:26: void Destroy(JNIEnv*env,
jobject obj);
On 2013/02/27 23:10:25, Yaron wrote:
> nit: insert space. also, this is uncalled and could potentially just be
dropped.

Done.

[nyquist, 2013-03-01 00:56:43]

tim/drew: PTAL. All comments should have been addressed by now.

[tim (not reviewing), 2013-03-01 08:09:15]

Some comments!

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:80: InvalidateAuthToken();
Is there a good reason we're doing so much work in the constructor, here?  Could
we have an Init function (like ProfileSyncService) that we call after
constructing this object (from your static Init below, I guess?)

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:122: return;
There's some non-trivial logic in here.  I think it warrants a test?  Can we add
a profile_sync_service_android_unittest?

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:154: void
ProfileSyncServiceAndroid::TokenAvailable(
Is this called to hand us tokens from the account manager or something?  Seems
like it should really be part of SigninManager[Android]?  When is it called
relative to SignInSync?

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:185:
sync_service_->HasSyncSetupCompleted())
multi-line if should have { }

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:386: jboolean
sync_everything,
Is it possible to put this parameters in a vector / struct? Something that won't
have to be changed every time we add an android type?

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:523:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
When is this object accessed from a non UI thread?

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:27: void NudgeSyncer(JNIEnv*
env,
Comment parameters, and threading.  We shouldn't use this class from a thread
that isn't the UI thread...

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:127: jboolean
IsMigrated(JNIEnv* env, jobject obj);
Define what "migrated" refers to here (we have lots of migrations :p).

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:196: int64 version,
What are these parameters?  The implementation references ipc::? Where do they
come from?

[nyquist, 2013-03-07 18:33:16]

tim: PTAL. Addressed all comments.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:80: InvalidateAuthToken();
On 2013/03/01 08:09:16, timsteele wrote:
> Is there a good reason we're doing so much work in the constructor, here? 
Could
> we have an Init function (like ProfileSyncService) that we call after
> constructing this object (from your static Init below, I guess?)

Done.
Moved out the business logic parts of this (sign in stuff) to an Init-method.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:122: return;
On 2013/03/01 08:09:16, timsteele wrote:
> There's some non-trivial logic in here.  I think it warrants a test?  Can we
add
> a profile_sync_service_android_unittest?

Discussed on chat. Will add tests when we have a better framework for it.
Filed http://crbug.com/180940

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:154: void
ProfileSyncServiceAndroid::TokenAvailable(
On 2013/03/01 08:09:16, timsteele wrote:
> Is this called to hand us tokens from the account manager or something?  Seems
> like it should really be part of SigninManager[Android]?  When is it called
> relative to SignInSync?

Yes, these token and sign in related things should definitely be part of
SigninManager[Android]. However, I would like to be able to punt on that for a
little bit until we get more code upstream, which should make it easier to clean
this up.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:185:
sync_service_->HasSyncSetupCompleted())
On 2013/03/01 08:09:16, timsteele wrote:
> multi-line if should have { }

Done.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:386: jboolean
sync_everything,
On 2013/03/01 08:09:16, timsteele wrote:
> Is it possible to put this parameters in a vector / struct? Something that
won't
> have to be changed every time we add an android type?

Done. After talking to the JNI overlords, they told me to use an autogenerated
bitmask enum.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:523:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2013/03/01 08:09:16, timsteele wrote:
> When is this object accessed from a non UI thread?

After looking through the code, this happens once, when Android account settings
have been changed. I will change that path to always use the UI thread.
I have removed this method, and renamed the old "inner" method to this method's
name.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_android.h (right):

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:27: void NudgeSyncer(JNIEnv*
env,
On 2013/03/01 08:09:16, timsteele wrote:
> Comment parameters, and threading.  We shouldn't use this class from a thread
> that isn't the UI thread...

Done.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:127: jboolean
IsMigrated(JNIEnv* env, jobject obj);
On 2013/03/01 08:09:16, timsteele wrote:
> Define what "migrated" refers to here (we have lots of migrations :p).

Done.

https://codereview.chromium.org/12313075/diff/22001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:196: int64 version,
On 2013/03/01 08:09:16, timsteele wrote:
> What are these parameters?  The implementation references ipc::? Where do they
> come from?

Done.

[nyquist, 2013-03-07 18:42:18]

dfalcantara: PTAL at
chrome/android/java[tests]/src/org/chromium/chrome/browser/identity/ and
chrome/android/java[tests]/src/org/chromium/chrome/browser/util/HashUtil[Test].java

[gone, 2013-03-07 19:17:45]

On 2013/03/07 18:42:18, nyquist wrote:
> dfalcantara: PTAL at
> chrome/android/java[tests]/src/org/chromium/chrome/browser/identity/ and
>
chrome/android/java[tests]/src/org/chromium/chrome/browser/util/HashUtil[Test].java

[gone, 2013-03-07 19:19:59]

LGTM for the parts you asked me to look at

https://codereview.chromium.org/12313075/diff/37001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/util/HashUtil.java
(right):

https://codereview.chromium.org/12313075/diff/37001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/util/HashUtil.java:24:
public static String getMd5Hash(@Nullable String salt, String text) {
Is it too late to have this thing to take in a Params object to avoid confusing
the order of the two string parameters?

[nyquist, 2013-03-07 21:19:57]

dfalcantara: Addressed the params-concern you had. Feel free to take a look.

https://codereview.chromium.org/12313075/diff/37001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/util/HashUtil.java
(right):

https://codereview.chromium.org/12313075/diff/37001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/util/HashUtil.java:24:
public static String getMd5Hash(@Nullable String salt, String text) {
On 2013/03/07 19:19:59, dfalcantara wrote:
> Is it too late to have this thing to take in a Params object to avoid
confusing
> the order of the two string parameters?

Done.

[gone, 2013-03-07 21:22:14]

lgtm

Thanks!

[tim (not reviewing), 2013-03-07 21:35:13]

On 2013/03/07 21:22:14, dfalcantara wrote:
> lgtm
> 
> Thanks!

LGTM, thanks

[commit-bot: I haz the power, 2013-03-07 21:48:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/12313075/43001

[commit-bot: I haz the power, 2013-03-07 21:48:41]

Presubmit check for 12313075-43001 failed and returned exit status 1.

INFO:root:Found 24 file(s).

INFO:PRESUBMIT:Skipping pylint: no matching changes.
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/build/android/PRESUBMIT.py
Running
/b/commit-queue/workdir/chromium/build/android/buildbot/tests/bb_run_bot_test.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/sync/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/sync/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/chrome.gyp
    chrome/chrome_browser.gypi

Following files should only contain deletions.
  build/android/findbugs_filter/findbugs_known_bugs.txt

Presubmit checks took 6.1s to calculate.

[nyquist, 2013-03-07 21:52:07]

sky: TBRing for trivial gyp changes.

[nyquist, 2013-03-07 22:22:21]

Committed patchset #10 manually as r186798.

[Yaron, 2013-03-12 02:26:49]

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:218:
profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, name);
On 2013/02/27 05:19:16, nyquist wrote:
> On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> > Should change our uses of kGoogleServicesUsername to
> > SigninManager::Get/SetAuthenticatedUsername()
> 
> Done.

Is it possible that this is causing crbug.com/181596

It seems like this causes us to only save kGoogleServicesLastUserName and not
kGoogleServicesUserName. At least, the code before set kGoogleServicesUserName
and SetAuthenticatedUsername only sets kGoogleServicesLastUserName. Where is
kGoogleServicesUserName supposed to be set?

[Yaron, 2013-03-12 02:31:14]

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service_android.cc (right):

https://codereview.chromium.org/12313075/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service_android.cc:218:
profile_->GetPrefs()->SetString(prefs::kGoogleServicesUsername, name);
On 2013/03/12 02:26:50, Yaron wrote:
> On 2013/02/27 05:19:16, nyquist wrote:
> > On 2013/02/25 16:23:35, Andrew T Wilson wrote:
> > > Should change our uses of kGoogleServicesUsername to
> > > SigninManager::Get/SetAuthenticatedUsername()
> > 
> > Done.
> 
> Is it possible that this is causing crbug.com/181596
> 
> It seems like this causes us to only save kGoogleServicesLastUserName and not
> kGoogleServicesUserName. At least, the code before set kGoogleServicesUserName
> and SetAuthenticatedUsername only sets kGoogleServicesLastUserName. Where is
> kGoogleServicesUserName supposed to be set?

It looks like it's set down in UnsuppressAndStart so this might be a red herring

[Andrew T Wilson (Slow), 2013-03-12 08:43:11]

On 2013/03/12 02:31:14, Yaron wrote:
> > It seems like this causes us to only save kGoogleServicesLastUserName and
not
> > kGoogleServicesUserName. At least, the code before set
kGoogleServicesUserName
> > and SetAuthenticatedUsername only sets kGoogleServicesLastUserName. Where is
> > kGoogleServicesUserName supposed to be set?
> 
> It looks like it's set down in UnsuppressAndStart so this might be a red
herring

Sigh. I thought SetAuthenticatedUsername() set that pref, but it doesn't. So you
have to set it manually.

I blame Tim, who refactored the code to create Get/SetAuthenticatedUsername(),
but never actually went the final step to make those the only interfaces to
kGoogleServicesUsername. TIIIIIIMMMMMMM!!!!!!

[tim (not reviewing), 2013-03-12 17:37:00]

On 2013/03/12 08:43:11, Andrew T Wilson wrote:
> On 2013/03/12 02:31:14, Yaron wrote:
> > > It seems like this causes us to only save kGoogleServicesLastUserName and
> not
> > > kGoogleServicesUserName. At least, the code before set
> kGoogleServicesUserName
> > > and SetAuthenticatedUsername only sets kGoogleServicesLastUserName. Where
is
> > > kGoogleServicesUserName supposed to be set?
> > 
> > It looks like it's set down in UnsuppressAndStart so this might be a red
> herring
> 
> Sigh. I thought SetAuthenticatedUsername() set that pref, but it doesn't. So
you
> have to set it manually.
> 
> I blame Tim, who refactored the code to create Get/SetAuthenticatedUsername(),
> but never actually went the final step to make those the only interfaces to
> kGoogleServicesUsername. TIIIIIIMMMMMMM!!!!!!

It is kinda my fault, I agree :( Some folks were unhappy with that proposal
though, because you can always set it via prefs:: even though you're not
"supposed to"; I still think it makes sense, but it seems hard to prevent
situations exactly like what the initial version of this patch here tried to do
:/  In any case, I'll see what I can do.