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

remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chromoting.jni;
 
 import android.app.Activity;
 import android.app.AlertDialog;
 import android.content.Context;
 import android.content.DialogInterface;
@@ skipping 168 matching lines @@
                 prefs.getString(hostId + "_id", ""), prefs.getString(hostId + "_secret", ""));
         sConnected = true;
     }
 
     /** Performs the native portion of the connection. */
     private static native void nativeConnect(String username, String authToken, String hostJid,
             String hostId, String hostPubkey, String pairId, String pairSecret);
 
     /** Severs the connection and cleans up. Called on the UI thread. */
     public static void disconnectFromHost() {
         if (!sConnected) return;

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

Nit: Elsewhere in this file, you've got braces for single-line conditionals.

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

Done.

 
         sConnectionListener.onConnectionState(ConnectionListener.State.CLOSED,
                 ConnectionListener.Error.OK);
 
+        disconnectFromHostWithoutNotification();
+    }
+
+    /** Same as disconnectFromHost() but without notifying the ConnectionListener. */
+    private static void disconnectFromHostWithoutNotification() {
+        if (!sConnected) return;
+
         nativeDisconnect();
         sConnectionListener = null;
         sConnected = false;
 
         // Drop the reference to free the Bitmap for GC.
         synchronized (sFrameLock) {
             sFrameBitmap = null;
         }
     }
 
     /** Performs the native portion of the cleanup. */
     private static native void nativeDisconnect();
 
     /** Reports whenever the connection status changes. Called on the UI thread. */
     @CalledByNative
-    private static void reportConnectionStatus(int state, int error) {
-        sConnectionListener.onConnectionState(ConnectionListener.State.fromValue(state),
-                ConnectionListener.Error.fromValue(error));
+    private static void reportConnectionStatus(int stateCode, int errorCode) {
+        ConnectionListener.State state = ConnectionListener.State.fromValue(stateCode);
+        ConnectionListener.Error error = ConnectionListener.Error.fromValue(errorCode);
+        sConnectionListener.onConnectionState(state, error);
+        if (state == ConnectionListener.State.FAILED || state == ConnectionListener.State.CLOSED) {
+            // Disconnect from the host here, otherwise the next time connectToHost() is called,
+            // it will try to disconnect, triggering an incorrect status notification.
+            disconnectFromHostWithoutNotification();

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

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.

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

Renamed the method (this touches some C++ files as well).

+        }
     }
 
     /** Prompts the user to enter a PIN. Called on the UI thread. */
     @CalledByNative
     private static void displayAuthenticationPrompt(boolean pairingSupported) {
         AlertDialog.Builder pinPrompt = new AlertDialog.Builder(sContext);
         pinPrompt.setTitle(sContext.getString(R.string.title_authenticate));
         pinPrompt.setMessage(sContext.getString(R.string.pin_message_android));
         pinPrompt.setIcon(android.R.drawable.ic_lock_lock);
 
         final View pinEntry = sContext.getLayoutInflater().inflate(R.layout.pin_dialog, null);
         pinPrompt.setView(pinEntry);
 
         final TextView pinTextView = (TextView)pinEntry.findViewById(R.id.pin_dialog_text);
         final CheckBox pinCheckBox = (CheckBox)pinEntry.findViewById(R.id.pin_dialog_check);
 
         if (!pairingSupported) {
             pinCheckBox.setChecked(false);
             pinCheckBox.setVisibility(View.GONE);
         }
 
         pinPrompt.setPositiveButton(
                 R.string.connect_button, new DialogInterface.OnClickListener() {
                     @Override
                     public void onClick(DialogInterface dialog, int which) {
                         Log.i("jniiface", "User provided a PIN code");
-                        nativeAuthenticationResponse(String.valueOf(pinTextView.getText()),
-                                pinCheckBox.isChecked(), Build.MODEL);
+                        if (sConnected) {
+                            nativeAuthenticationResponse(String.valueOf(pinTextView.getText()),
+                                    pinCheckBox.isChecked(), Build.MODEL);
+                        }

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

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]

Done.

                     }
                 });
 
         pinPrompt.setNegativeButton(
                 R.string.cancel, new DialogInterface.OnClickListener() {
                     @Override
                     public void onClick(DialogInterface dialog, int which) {
                         Log.i("jniiface", "User canceled pin entry prompt");
                         disconnectFromHost();
                     }
@@ skipping 200 matching lines @@
     /** Pops up a third party login page to fetch the token required for authentication. */
     @CalledByNative
     public static void fetchThirdPartyToken(String tokenUrl, String clientId, String scope) {
         Chromoting app = (Chromoting) sContext;
         app.fetchThirdPartyToken(tokenUrl, clientId, scope);
     }
 
     /**
      * Notify the native code to continue authentication with the |token| and the |sharedSecret|.
      */
-    public static native void nativeOnThirdPartyTokenFetched(String token, String sharedSecret);
+    public static void onThirdPartyTokenFetched(String token, String sharedSecret) {
+        if (!sConnected) {
+            return;
+        }
+
+        nativeOnThirdPartyTokenFetched(token, sharedSecret);
+    }
+
+    /** Passes authentication data to the native handling code. */
+    private static native void nativeOnThirdPartyTokenFetched(String token, String sharedSecret);
 }