Get OAuth2TokenService working on Android.

Get OAuth2TokenService working on Android.

In order to get OAuth tokens on Android, we have to call out to Java. This CL
makes it possible. It's not the ideal solution but works for now.

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

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

[Patrick Dubroy, 2013-03-25 17:58:54]

Please take a look.

[Nicolas Zea, 2013-03-25 18:13:17]

sync LGTM with some nits

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

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:192: // Allocate a copy of
the callback on the heap, because we need a pointer to
nit: "we" here is ambiguous. Perhaps just "because JNI needs a callback pointer
it can plumb back when the fetch completes"

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

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.h:30: const
GoogleServiceAuthError,const std::string,const base::Time)>
Comment about what these params are.

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.h:198: // param is a pointer to
the callback. |invalid_auth_token| may be null,
nit: not a pointer. Maybe just say that |callback| is invoked asynchronously
once the token fetch completes.

[Patrick Dubroy, 2013-03-25 18:52:55]

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

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.cc:192: // Allocate a copy of
the callback on the heap, because we need a pointer to
On 2013/03/25 18:13:18, Nicolas Zea wrote:
> nit: "we" here is ambiguous. Perhaps just "because JNI needs a callback
pointer
> it can plumb back when the fetch completes"

Done.

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

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.h:198: // param is a pointer to
the callback. |invalid_auth_token| may be null,
On 2013/03/25 18:13:18, Nicolas Zea wrote:
> nit: not a pointer. Maybe just say that |callback| is invoked asynchronously
> once the token fetch completes. 

Done.

[Roger Tawa OOO till Jul 10th, 2013-03-25 19:14:35]

Hi Patrick, a few questions below.

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/history/web...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/history/web...
chrome/browser/history/web_history_service.cc:79: oauth_scopes, this,
access_token_);
Is there a reason to have a method to invalidate and request a new token in one
call instead of exposing a method that simply invalidates a token?

Should access_token_ be cleared here?

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/history/web...
chrome/browser/history/web_history_service.cc:93: Start(true);
Seems like you could just invalidate the token here before calling Start(). 
Then you would't need to change Start() at all?  It does not seem like you are
calling Start() with true anywhere else.

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

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_android.h:30: const
GoogleServiceAuthError,const std::string,const base::Time)>
Should these be const refs?

[Roger Tawa OOO till Jul 10th, 2013-03-25 20:47:20]

lgtm

https://codereview.chromium.org/12880014/diff/18001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/12880014/diff/18001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:194:
if (account == null) return;
return on next line?

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

https://codereview.chromium.org/12880014/diff/21001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:191: // Call into java to
invalidate the current token and get a new one.
is this comment correct?

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

https://codereview.chromium.org/12880014/diff/21001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:31: FetchOAuth2TokenCallback;
Document args?

[nyquist, 2013-03-25 21:01:21]

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:8:
import android.accounts.AccountManager;
Should not import this.

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:224:
public void invalidateOAuth2AuthToken(String scope, String access_token) {
camelCase

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:225:
AccountManager.get(mContext).invalidateAuthToken(scope, access_token);
Do not use the AccountManager directly. Always go through the
AccountManagerHelper. Otherwise it is a pain to test this. We don't want our
tests to hit the real auth backends.

Just add a simple method to AccountManagerHelper that invalidates an auth token.

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:238:
String username, String scope, final int nativeCallback) {
Nit: Does this fit on the line above?

[Patrick Dubroy, 2013-03-25 21:15:49]

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/history/web...
File chrome/browser/history/web_history_service.cc (right):

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/history/web...
chrome/browser/history/web_history_service.cc:79: oauth_scopes, this,
access_token_);
On 2013/03/25 19:14:35, Roger Tawa wrote:
> Is there a reason to have a method to invalidate and request a new token in
one
> call instead of exposing a method that simply invalidates a token?
> 
> Should access_token_ be cleared here?

Done.

https://codereview.chromium.org/12880014/diff/5001/chrome/browser/history/web...
chrome/browser/history/web_history_service.cc:93: Start(true);
On 2013/03/25 19:14:35, Roger Tawa wrote:
> Seems like you could just invalidate the token here before calling Start(). 
> Then you would't need to change Start() at all?  It does not seem like you are
> calling Start() with true anywhere else.

Done.

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java
(right):

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:8:
import android.accounts.AccountManager;
On 2013/03/25 21:01:21, nyquist wrote:
> Should not import this.

Done.

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:224:
public void invalidateOAuth2AuthToken(String scope, String access_token) {
On 2013/03/25 21:01:21, nyquist wrote:
> camelCase

Done.

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:225:
AccountManager.get(mContext).invalidateAuthToken(scope, access_token);
On 2013/03/25 21:01:21, nyquist wrote:
> Do not use the AccountManager directly. Always go through the
> AccountManagerHelper. Otherwise it is a pain to test this. We don't want our
> tests to hit the real auth backends.
> 
> Just add a simple method to AccountManagerHelper that invalidates an auth
token.

Done.

https://codereview.chromium.org/12880014/diff/21001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/sync/ProfileSyncService.java:238:
String username, String scope, final int nativeCallback) {
On 2013/03/25 21:01:21, nyquist wrote:
> Nit: Does this fit on the line above?

Done.

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

https://codereview.chromium.org/12880014/diff/21001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.cc:191: // Call into java to
invalidate the current token and get a new one.
On 2013/03/25 20:47:20, Roger Tawa wrote:
> is this comment correct?

Nope, done.

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

https://codereview.chromium.org/12880014/diff/21001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_android.h:31: FetchOAuth2TokenCallback;
On 2013/03/25 20:47:20, Roger Tawa wrote:
> Document args?

Done.

[Patrick Dubroy, 2013-03-25 21:22:17]

+brettw for owners rubber stamp on chrome/browser/history.

[nyquist, 2013-03-25 21:42:35]

lgtm

[brettw, 2013-03-25 23:07:31]

owners lgtm

[Patrick Dubroy, 2013-03-26 00:49:18]

Committed patchset #8 manually as r190531 (presubmit successful).

[frankf, 2013-03-26 05:47:22]

Why was this committed when unit tests failed in android_dbg_trigerred_tests?

On 2013/03/26 00:49:18, dubroy wrote:
> Committed patchset #8 manually as r190531 (presubmit successful).

[Patrick Dubroy, 2013-03-26 08:46:00]

Committed patchset #9 manually as r190600 (presubmit successful).