Pull authentication UX code out of JniInterface

Pull authentication UX code out of JniInterface

UX code does not belong in the JniInterface layer. This is a first step
towards refactoring the JNI layer away from static methods, into a
separate object whose lifetime is controlled by Java, which can be
mocked for testing.

Committed: https://crrev.com/d09f63e285e28e90743c16332b06bc89d17de1a5
Cr-Commit-Position: refs/heads/master@{#321187}

[Lambros, 2015-03-05 00:31:19]

lambroslambrou@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2015-03-09 18:14:53]

Thanks for the cleanup!

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java
(right):

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:119:
mApplicationContext.fetchThirdPartyToken(tokenUrl, clientId, scope);
I think you can move implementation of this function from Chromoting class here.
Just need to pass host.getTokenUrlPatterns() to this class.

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:147:
sAuthenticator = new SessionAuthenticator(context);
Ideally SessionAuthenticator should be an interface and JNI layer shouldn't be
responsible for creating it.

I think it's better to pass authenticator from the activity and it belongs to
connectoToHost(), not here. This would also allow to remove dependency on
Chromoting class

[Lambros, 2015-03-14 00:42:17]

lambroslambrou@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Lambros, 2015-03-14 00:42:18]

+kelvinp: Can you look at the ThirdParty changes, make sure I haven't done
anything silly?

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java
(right):

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:119:
mApplicationContext.fetchThirdPartyToken(tokenUrl, clientId, scope);
On 2015/03/09 18:14:53, Sergey Ulanov wrote:
> I think you can move implementation of this function from Chromoting class
here.
> Just need to pass host.getTokenUrlPatterns() to this class.

Done.

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/981783003/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:147:
sAuthenticator = new SessionAuthenticator(context);
On 2015/03/09 18:14:53, Sergey Ulanov wrote:
> Ideally SessionAuthenticator should be an interface and JNI layer shouldn't be
> responsible for creating it.
> 
> I think it's better to pass authenticator from the activity and it belongs to
> connectoToHost(), not here. This would also allow to remove dependency on
> Chromoting class

Done, except for making SessionAuthenticator an interface. Can that wait, or
shall I do it here?

[kelvinp, 2015-03-16 17:53:09]

lgtm on the ThirdPartyTokenFetcher Changes.

[Sergey Ulanov, 2015-03-16 19:56:10]

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java
(right):

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:53:
mTokenFetcher = new ThirdPartyTokenFetcher(context, host.getTokenUrlPatterns(),
callback);
Does this need to be in the constructor? Can it be moved to
fetchThirdPartyToken()?

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:144:
public void handleTokenFetched(Intent intent) {
I think it would be better to call this method onNewIntent(). Otherwise it's
confusing because this call is ignored when mTokenFetcher is null. The new
intent may not be related to third-party auth.

[Lambros, 2015-03-18 01:01:14]

ptal

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java
(right):

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:53:
mTokenFetcher = new ThirdPartyTokenFetcher(context, host.getTokenUrlPatterns(),
callback);
On 2015/03/16 19:56:09, Sergey Ulanov wrote:
> Does this need to be in the constructor? Can it be moved to
> fetchThirdPartyToken()?

Done.

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:53:
mTokenFetcher = new ThirdPartyTokenFetcher(context, host.getTokenUrlPatterns(),
callback);
On 2015/03/16 19:56:09, Sergey Ulanov wrote:
> Does this need to be in the constructor? Can it be moved to
> fetchThirdPartyToken()?

Done.

https://codereview.chromium.org/981783003/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/SessionAuthenticator.java:144:
public void handleTokenFetched(Intent intent) {
On 2015/03/16 19:56:09, Sergey Ulanov wrote:
> I think it would be better to call this method onNewIntent(). Otherwise it's
> confusing because this call is ignored when mTokenFetcher is null. The new
> intent may not be related to third-party auth.

Done.

[Sergey Ulanov, 2015-03-18 17:55:03]

lgtm

https://codereview.chromium.org/981783003/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/981783003/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:93: /** Object
for handling the Intent received when using third-party session authentication.
*/
I don't think this is a good description. Handling new intents for third-party
auth is only one of the SessionAuthenticator's responsibilities. I suggest
changing this comment to something like "Helper used by SessionConnection for
session authentication. Receives onNewIntent() notifications to handle
third-party authentication."

[Lambros, 2015-03-18 18:45:20]

https://codereview.chromium.org/981783003/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/981783003/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:93: /** Object
for handling the Intent received when using third-party session authentication.
*/
On 2015/03/18 17:55:03, Sergey Ulanov wrote:
> I don't think this is a good description. Handling new intents for third-party
> auth is only one of the SessionAuthenticator's responsibilities. I suggest
> changing this comment to something like "Helper used by SessionConnection for
> session authentication. Receives onNewIntent() notifications to handle
> third-party authentication."

Done.

[Lambros, 2015-03-18 18:45:23]

The CQ bit was checked by lambroslambrou@chromium.org

[Lambros, 2015-03-18 18:45:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
kelvinp@chromium.org, sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/981783003/#ps80001 (title:
"Fix comment")

[commit-bot: I haz the power, 2015-03-18 18:46:03]

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

[commit-bot: I haz the power, 2015-03-18 19:17:42]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-18 19:18:38]

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