Third Party Authentication for Android Part II - Java Native Interface plumbing

Third Party Authentication for Android Part II - Java Native Interface plumbing

This CL implements the plumbing required for the Java android client to communicate with the C++ client via Java Native Interface.

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

[Sergey Ulanov, 2014-06-07 01:32:46]

lgtm when my comments are addressed

https://codereview.chromium.org/319273003/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/319273003/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:443: //
TODO(kelvinp): Create a intent to fetch the token from the browser
call nativeOnThirdPartyTokenFetched() with empty tokens to indicated that it's
not implemented it. Otherwise the instnace would keep waiting for nothing

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:118: "ThirdPartyAuth",
maybe add a tag to be used for all log messages in this file (e.g.
"ChromotingJNI")?

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:129: };
don't need semicolon

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:146: "Ignored
OnThirdPartyTokenFetched without a pending fetch.");
nit: add () after function name

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:148: };
don't need semicolon

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.h:190: // Weak reference to this
instance, used for task posting.
don't need this comment.

[Lambros, 2014-06-07 01:38:12]

lgtm with comments

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:114:
base::WeakPtr<TokenFetcherProxy> token_fetcher_proxy) {
Add
DCHECK(jni_runtime_->network_task_runner()->BelongsToCurrentThread());

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:319:
scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>
Maybe add 'using' declaration for
remoting::protocol::ThirdPartyClientAuthenticator?

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_runtime.h (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_runtime.h:100: // authentication.
Add "Call on UI thread."
I think these methods are grouped together by which thread they are called on,
so perhaps move this just below CommitPairingCredentials()?

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_runtime.h:103: const std::string& scope);
Same here. Make sure the ordering of methods in .cc file follows the header file
(assuming it does so already).

[kelvinp, 2014-06-09 18:42:33]

https://codereview.chromium.org/319273003/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/319273003/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:443: //
TODO(kelvinp): Create a intent to fetch the token from the browser
On 2014/06/07 01:32:45, Sergey Ulanov wrote:
> call nativeOnThirdPartyTokenFetched() with empty tokens to indicated that it's
> not implemented it. Otherwise the instnace would keep waiting for nothing

This code is currently not being run.  We need to add "third_party" in the
client authentication in order to enable it.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:114:
base::WeakPtr<TokenFetcherProxy> token_fetcher_proxy) {
On 2014/06/07 01:38:12, Lambros wrote:
> Add
> DCHECK(jni_runtime_->network_task_runner()->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:118: "ThirdPartyAuth",
On 2014/06/07 01:32:45, Sergey Ulanov wrote:
> maybe add a tag to be used for all log messages in this file (e.g.
> "ChromotingJNI")?

The convention in the current file is to log messages by feature (e.g. stats on
line 470).  I am keeping the existing convention.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:129: };
On 2014/06/07 01:32:45, Sergey Ulanov wrote:
> don't need semicolon

Done.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:146: "Ignored
OnThirdPartyTokenFetched without a pending fetch.");
On 2014/06/07 01:32:45, Sergey Ulanov wrote:
> nit: add () after function name

Done.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:148: };
On 2014/06/07 01:32:45, Sergey Ulanov wrote:
> don't need semicolon

Done.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.cc:319:
scoped_ptr<protocol::ThirdPartyClientAuthenticator::TokenFetcher>
On 2014/06/07 01:38:12, Lambros wrote:
> Maybe add 'using' declaration for
> remoting::protocol::ThirdPartyClientAuthenticator?

The using directive doesn't work with template arguments.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.h:190: // Weak reference to this
instance, used for task posting.
On 2014/06/07 01:32:45, Sergey Ulanov wrote:
> don't need this comment.

Done.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_runtime.h (right):

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_runtime.h:100: // authentication.
On 2014/06/07 01:38:12, Lambros wrote:
> Add "Call on UI thread."
> I think these methods are grouped together by which thread they are called on,
> so perhaps move this just below CommitPairingCredentials()?

Done.

https://codereview.chromium.org/319273003/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_runtime.h:103: const std::string& scope);
On 2014/06/07 01:38:12, Lambros wrote:
> Same here. Make sure the ordering of methods in .cc file follows the header
file
> (assuming it does so already).

Done.

[kelvinp, 2014-06-09 18:42:38]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-06-09 18:44:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kelvinp@chromium.org/319273003/60001

[commit-bot: I haz the power, 2014-06-10 09:00:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-10 15:26:03]

Change committed as 276038