Chromoting: Synchronize connected/disconnected state between Java/C++

Chromoting: Synchronize connected/disconnected state between Java/C++

If the connection was dropped (network error or similar), the session
was not being cleaned up straight away. Instead, the cleanup was
delayed until the next time Java tried to connect, by which time it
had already popped open the progress dialog. This raised an
incorrect disconnection notification that closed the progress
dialog.

This CL ensures that disconnection causes an immediate notification to
Java, and the state is kept consistent between Java and C++ code.

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

[Lambros, 2014-07-23 01:35:22]

This seems a bit ugly as it stands. Feel free to suggest better ways of doing
this.

[Sergey Ulanov, 2014-07-24 02:47:03]

I'd prefer moving more logic to the java layer. Ideally Java layer should
control lifetime of the client instance. 

Also currently our JNI layer looks really bad and we should try
refactoring/rewriting it in the future. E.g. distinction between
ChromotingJniInstance and ChromotingJniRuntime doesn't make much sense. We
should have just ChromotingJniInstance that's called from Java. Right now
ChromotingJniRuntime is essentially a wrapper that holds static reference to the
current instance. Also JniInterface is static, which also doesn't make much
sense because it has to be reused across multiple session. It would be better if
we had org.chromium.chromoting.jni.Client that would instantiate
ChromotingJniInstance internally and then delete it at the end of each session.
We could use jni_generator from base/android. Take a look at
base/android/jni_generator/sample_for_tests.h - it seems to be much cleaner way
to implement JNI interfaces compared to what we have now.
Obviously it doesn't have to happen in this CL.

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

https://codereview.chromium.org/407403002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:212:
switch (state) {
Don't really need switch statement here. if would be sufficient.

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
File remoting/client/jni/chromoting_jni_runtime.cc (right):

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_runtime.cc:30:
remoting::ChromotingJniInstance* GetSession() {
nit: Move this inside the remoting namespace below, then you wouldn't need to
specify remoting:: here.

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_runtime.cc:104: if (GetSession()) {
I think it's better to check that the session in Java layer and only then call
this method.

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_runtime.cc:266: DisconnectFromHost();
Can the Java layer call Disconnect() when the host is disconnected?

[Lambros, 2014-07-26 00:42:35]

I agree with that approach. We're already using jni_generator but we could make
better use of it!

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

https://codereview.chromium.org/407403002/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:212:
switch (state) {
On 2014/07/24 02:47:02, Sergey Ulanov wrote:
> Don't really need switch statement here. if would be sufficient.

Done.

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
File remoting/client/jni/chromoting_jni_runtime.cc (right):

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_runtime.cc:30:
remoting::ChromotingJniInstance* GetSession() {
On 2014/07/24 02:47:02, Sergey Ulanov wrote:
> nit: Move this inside the remoting namespace below, then you wouldn't need to
> specify remoting:: here.

Removed.

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_runtime.cc:104: if (GetSession()) {
On 2014/07/24 02:47:02, Sergey Ulanov wrote:
> I think it's better to check that the session in Java layer and only then call
> this method.

Done.

https://codereview.chromium.org/407403002/diff/1/remoting/client/jni/chromoti...
remoting/client/jni/chromoting_jni_runtime.cc:266: DisconnectFromHost();
On 2014/07/24 02:47:02, Sergey Ulanov wrote:
> Can the Java layer call Disconnect() when the host is disconnected?

Done.

[Jamie, 2014-07-30 17:22:04]

LGTM once my comments are addressed.

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

https://codereview.chromium.org/407403002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:189: if
(!sConnected) return;
Nit: Elsewhere in this file, you've got braces for single-line conditionals.

https://codereview.chromium.org/407403002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:223:
disconnectFromHostWithoutNotification();
I don't like the fact that a method called reportConnectionStatus also closes
the connection. You could rename the method (handleStateChange?) or move the
disconnect out of this method.

https://codereview.chromium.org/407403002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:254: }
I think we should show an error in this case. It would make the code more
readable and be better UX.

[Lambros, 2014-08-12 01:27:35]

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

https://codereview.chromium.org/407403002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:189: if
(!sConnected) return;
On 2014/07/30 17:22:04, Jamie wrote:
> Nit: Elsewhere in this file, you've got braces for single-line conditionals.

Done.

https://codereview.chromium.org/407403002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:223:
disconnectFromHostWithoutNotification();
On 2014/07/30 17:22:04, Jamie wrote:
> I don't like the fact that a method called reportConnectionStatus also closes
> the connection. You could rename the method (handleStateChange?) or move the
> disconnect out of this method.
Renamed the method (this touches some C++ files as well).

https://codereview.chromium.org/407403002/diff/20001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:254: }
On 2014/07/30 17:22:04, Jamie wrote:
> I think we should show an error in this case. It would make the code more
> readable and be better UX.

Done.

[Lambros, 2014-08-12 01:27:40]

The CQ bit was checked by lambroslambrou@chromium.org

[commit-bot: I haz the power, 2014-08-12 01:33:37]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/407403002/...

[commit-bot: I haz the power, 2014-08-12 14:50:59]

Change committed as 288967