Add sign-in and token retrieval to Blimp

Add sign-in and token retrieval to Blimp

- Add a basic flow to request for an account on the phone and create an auth token.
- Add exponential backoff retry support in case this fails.
- Add unit tests for the retry backoff.

BUG=536249
Committed: https://crrev.com/63d8da2dfcb12948f41efc91dbf7ad8a05c3e70d
Cr-Commit-Position: refs/heads/master@{#352452}

[David Trainor- moved to gerrit, 2015-09-28 17:29:16]

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

[David Trainor- moved to gerrit, 2015-09-28 17:29:16]

ptal!

[nyquist, 2015-09-29 23:38:40]

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:26:
private static final String TAG = "cr_Blimp";
Nit: Just 'Blimp' nowadays apparently.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File
blimp/client/android/java/src/org/chromium/blimp/auth/RetryingTokenSource.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/RetryingTokenSource.java:55:
// Reset all exponential backoff states.
Do we need any thread safety for the public methods in this class?

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/RetryingTokenSource.java:115:
private void getTokenWithBackoff() {
Could we make this a bit randomized? See
https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...
as an example.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java:30: void
onTokenUnavailable(boolean isTransient);
If the error isTransient, maybe we could comment somewhere explaining what the
RetryingTokenSource will do?

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java:53: void
setCallback(Callback callback);
Instead of setCallback, could we pass in the callback to getToken(Callback)?
Chances are, there will be the same object that calls setCallback(...) and
getToken(), so it could just pass in |this| or something anyway to
getToken(Callback).

You might have to keep a little bit more state in the TokenSource though, since
you could potentially get multiple calls to getToken(Callback). Either that, or
just keep on passing the |callback| object around.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java:85: void
onAccountSelected(Intent data);
Would it be clearer for this interface if either:
A) We would add a comment about there having to be a key
AccountManager.KEY_ACCOUNT_NAME that contains the |account.name|?
B) This method takes |String accountName| as a parameter directly.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java:36:
private static final String ACCOUNT_SCOPE =
Nit: OAUTH2_SCOPE
Could you also add a comment explaining why the scope is prefixed with
'oauth2:'?

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java:39:
private static final String TAG = "cr_BlimpTokenSource";
Nit: s/cr\_//

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java:160:
protected void onPostExecute(String result) {
Nit: s/result/token/

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/javate...
File
blimp/client/android/javatests/src/org/chromium/blimp/auth/MockTokenSource.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/javate...
blimp/client/android/javatests/src/org/chromium/blimp/auth/MockTokenSource.java:24:
private TokenSource.Callback mCallback;
Nit: Could you put final members before non-final members?

[David Trainor- moved to gerrit, 2015-10-05 15:19:08]

addressed comments. ptal.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/BlimpRendererActivity.java:26:
private static final String TAG = "cr_Blimp";
On 2015/09/29 23:38:40, nyquist wrote:
> Nit: Just 'Blimp' nowadays apparently.

Done.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File
blimp/client/android/java/src/org/chromium/blimp/auth/RetryingTokenSource.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/RetryingTokenSource.java:55:
// Reset all exponential backoff states.
On 2015/09/29 23:38:40, nyquist wrote:
> Do we need any thread safety for the public methods in this class?

Done.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/RetryingTokenSource.java:115:
private void getTokenWithBackoff() {
On 2015/09/29 23:38:40, nyquist wrote:
> Could we make this a bit randomized? See
>
https://chromium.googlesource.com/chromium/src/+/master/chrome/android/java/s...
> as an example.

Done.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java:30: void
onTokenUnavailable(boolean isTransient);
On 2015/09/29 23:38:40, nyquist wrote:
> If the error isTransient, maybe we could comment somewhere explaining what the
> RetryingTokenSource will do?

isTransient could also be handled by any TokenSource implementer.  Ideally the
RetryingTokenSource would actually expose transient failures after X retries. 
I'll comment on it in the RetryingTokenSource though.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java:53: void
setCallback(Callback callback);
On 2015/09/29 23:38:40, nyquist wrote:
> Instead of setCallback, could we pass in the callback to getToken(Callback)?
> Chances are, there will be the same object that calls setCallback(...) and
> getToken(), so it could just pass in |this| or something anyway to
> getToken(Callback).
> 
> You might have to keep a little bit more state in the TokenSource though,
since
> you could potentially get multiple calls to getToken(Callback). Either that,
or
> just keep on passing the |callback| object around.

Sadly this gets more complicated.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSource.java:85: void
onAccountSelected(Intent data);
On 2015/09/29 23:38:40, nyquist wrote:
> Would it be clearer for this interface if either:
> A) We would add a comment about there having to be a key
> AccountManager.KEY_ACCOUNT_NAME that contains the |account.name|?
> B) This method takes |String accountName| as a parameter directly.

I'd rather it be explicit that the user should call startActivityForResult() on
the Callback intent and pass the returned intent directly to this class, that
way I don't expose some of the intent internals.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
File blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java:36:
private static final String ACCOUNT_SCOPE =
On 2015/09/29 23:38:40, nyquist wrote:
> Nit: OAUTH2_SCOPE
> Could you also add a comment explaining why the scope is prefixed with
> 'oauth2:'?

Done.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java:39:
private static final String TAG = "cr_BlimpTokenSource";
On 2015/09/29 23:38:40, nyquist wrote:
> Nit: s/cr\_//

Done.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/java/s...
blimp/client/android/java/src/org/chromium/blimp/auth/TokenSourceImpl.java:160:
protected void onPostExecute(String result) {
On 2015/09/29 23:38:40, nyquist wrote:
> Nit: s/result/token/

Done.

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/javate...
File
blimp/client/android/javatests/src/org/chromium/blimp/auth/MockTokenSource.java
(right):

https://codereview.chromium.org/1373943002/diff/1/blimp/client/android/javate...
blimp/client/android/javatests/src/org/chromium/blimp/auth/MockTokenSource.java:24:
private TokenSource.Callback mCallback;
On 2015/09/29 23:38:40, nyquist wrote:
> Nit: Could you put final members before non-final members?

Done.

[nyquist, 2015-10-05 18:05:26]

lgtm

[David Trainor- moved to gerrit, 2015-10-05 20:03:17]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2015-10-05 20:05:20]

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

[commit-bot: I haz the power, 2015-10-05 21:21:20]

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

[commit-bot: I haz the power, 2015-10-05 21:21:21]

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

[David Trainor- moved to gerrit, 2015-10-05 22:41:00]

The CQ bit was checked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2015-10-05 22:41:01]

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/1373943002/#ps80001
(title: "Fixed DEPS issue")

[commit-bot: I haz the power, 2015-10-05 22:42:11]

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

[commit-bot: I haz the power, 2015-10-05 22:48:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-05 22:49:27]

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