Refactor Chromoting.java to pull out some nested classes.

remoting/android/java/src/org/chromium/chromoting/Chromoting.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;
 
 import android.accounts.Account;
 import android.accounts.AccountManager;
 import android.accounts.AccountManagerCallback;
 import android.accounts.AccountManagerFuture;
 import android.accounts.AuthenticatorException;
 import android.accounts.OperationCanceledException;
 import android.app.Activity;
-import android.content.Context;
 import android.content.Intent;
 import android.content.SharedPreferences;
 import android.os.Bundle;
 import android.os.Handler;
 import android.os.HandlerThread;
-import android.text.Html;
 import android.util.Log;
 import android.view.Menu;
 import android.view.MenuItem;
-import android.view.View;
-import android.view.ViewGroup;
 import android.widget.ArrayAdapter;
 import android.widget.ListView;
 import android.widget.TextView;
 import android.widget.Toast;
 
 import org.chromium.chromoting.jni.JniInterface;
 import org.json.JSONArray;
 import org.json.JSONException;
 import org.json.JSONObject;
 
@@ skipping 11 matching lines @@
     private static final String ACCOUNT_TYPE = "com.google";
 
     /** Scopes at which the authentication token we request will be valid. */
     private static final String TOKEN_SCOPE = "oauth2:https://www.googleapis.com/auth/chromoting " +
             "https://www.googleapis.com/auth/googletalk";
 
     /** Path from which to download a user's host list JSON object. */
     private static final String HOST_LIST_PATH =
             "https://www.googleapis.com/chromoting/v1/@me/hosts?key=";
 
-    /** Color to use for hosts that are online. */
-    private static final String HOST_COLOR_ONLINE = "green";
-
-    /** Color to use for hosts that are offline. */
-    private static final String HOST_COLOR_OFFLINE = "red";
+    /** Lock to protect |mAccount| and |mToken|. */
+    // TODO(lambroslambrou): |mHosts| needs to be protected as well.
+    private Object mLock = new Object();

[Sergey Ulanov, 2013/12/28 02:17:30]

Do we really need this lock? It won't be necessary if we access mAccount and
mToken only on the UI/main thread.

[Lambros, 2013/12/30 21:46:35]

We still need the network thread. I don't know if that thread needs to access
these references, or whether we could pass copies of these data. I guess a lock
is probably simpler for now.

 
     /** User's account details. */
     private Account mAccount;
 
     /** Account auth token. */
     private String mToken;
 
     /** List of hosts. */
     private JSONArray mHosts;
 
@@ skipping 32 matching lines @@
         HandlerThread thread = new HandlerThread("auth_callback");
         thread.start();
         mNetwork = new Handler(thread.getLooper());
 
         SharedPreferences prefs = getPreferences(MODE_PRIVATE);
         if (prefs.contains("account_name") && prefs.contains("account_type")) {
             // Perform authentication using saved account selection.
             mAccount = new Account(prefs.getString("account_name", null),
                     prefs.getString("account_type", null));
             AccountManager.get(this).getAuthToken(mAccount, TOKEN_SCOPE, null, this,
                     new HostListDirectoryGrabber(this), mNetwork);

[Sergey Ulanov, 2013/12/28 02:17:30]

The last parameter here specifies the thread on which the callback will be
called. If you pass null it will be called on the main thread. Then you wouldn't
need the lock.

[Lambros, 2013/12/30 21:46:35]

We still need the lock, regardless of what we pass to AccountManager. Our
callback performs a network operation to grab the host-list, and that needs to
run on a non-UI thread.

             if (mAccountSwitcher != null) {
                 mAccountSwitcher.setTitle(mAccount.name);
             }
         } else {
             // Request auth callback once user has chosen an account.
             Log.i("auth", "Requesting auth token from system");
             AccountManager.get(this).getAuthTokenByFeatures(
                     ACCOUNT_TYPE,
                     TOKEN_SCOPE,
                     null,
@@ skipping 56 matching lines @@
         }
         else {
             // The refresh button simply makes use of the currently-chosen account.
             AccountManager.get(this).getAuthToken(mAccount, TOKEN_SCOPE, null, this,
                     new HostListDirectoryGrabber(this), mNetwork);
         }
 
         return true;
     }
 
+    /** Called when the user taps on a host entry. */
+    public void connectToHost(JSONObject host) {
+        try {
+            synchronized (mLock) {
+                JniInterface.connectToHost(mAccount.name, mToken, host.getString("jabberId"),
+                        host.getString("hostId"), host.getString("publicKey"),
+                        new Runnable() {
+                                @Override
+                                public void run() {
+                                    startActivity(new Intent(Chromoting.this, Desktop.class));
+                                }
+                        });
+            }
+        } catch (JSONException ex) {
+            Log.w("host", ex);
+            Toast.makeText(this, getString(R.string.error_reading_host),
+                    Toast.LENGTH_LONG).show();
+            // Close the application.
+            finish();
+        }
+    }
+
     /**
      * Processes the authentication token once the system provides it. Once in possession of such a
      * token, attempts to request a host list from the directory server. In case of a bad response,
      * this is retried once in case the system's cached auth token had expired.
      */
     private class HostListDirectoryGrabber implements AccountManagerCallback<Bundle> {

[Sergey Ulanov, 2013/12/28 02:17:30]

While you are removing nested classes, you can get rid of this one too. I think
it can be replaced with a single method and an inline implementation of
AccountManagerCallback<>.

[Lambros, 2013/12/30 21:46:35]

The problem is that the nested class has some internal state: boolean
mAlreadyTried, and its run() method invokes the AccountManager a second time,
passing "this" as a callback parameter (with mAlreadyTried set to true). The
"single method" that you propose, wouldn't work as a method of the outer class,
as its "this" reference would be the wrong thing.

         /** Whether authentication has already been attempted. */
         private boolean mAlreadyTried;
 
         /** Communication with the screen. */
         private Activity mUi;
 
         /** Constructor. */
         public HostListDirectoryGrabber(Activity ui) {
             mAlreadyTried = false;
             mUi = ui;
         }
 
         /**
          * Retrieves the host list from the directory server. This method performs
          * network operations and must be run an a non-UI thread.
          */
         @Override
         public void run(AccountManagerFuture<Bundle> future) {
             Log.i("auth", "User finished with auth dialogs");
             try {
                 // Here comes our auth token from the Android system.
                 Bundle result = future.getResult();
                 String accountName = result.getString(AccountManager.KEY_ACCOUNT_NAME);
                 String accountType = result.getString(AccountManager.KEY_ACCOUNT_TYPE);
                 String authToken = result.getString(AccountManager.KEY_AUTHTOKEN);
                 Log.i("auth", "Received an auth token from system");
 
-                synchronized (mUi) {
+                synchronized (mLock) {
                     mAccount = new Account(accountName, accountType);
                     mToken = authToken;
                     getPreferences(MODE_PRIVATE).edit().putString("account_name", accountName).
                             putString("account_type", accountType).apply();
                 }
 
                 // Send our HTTP request to the directory server.
                 URLConnection link =
                         new URL(HOST_LIST_PATH + JniInterface.nativeGetApiKey()).openConnection();
                 link.addRequestProperty("client_id", JniInterface.nativeGetClientId());
@@ skipping 20 matching lines @@
                 // Assemble error message to display to the user.
                 String explanation = getString(R.string.error_unknown);
                 if (ex instanceof OperationCanceledException) {
                     explanation = getString(R.string.error_auth_canceled);
                 } else if (ex instanceof AuthenticatorException) {
                     explanation = getString(R.string.error_no_accounts);
                 } else if (ex instanceof IOException) {
                     if (!mAlreadyTried) {
                         // This was our first connection attempt.
 
-                        synchronized (mUi) {
+                        synchronized (mLock) {
                             if (mAccount != null) {
                                 // We got an account, but couldn't log into it. We'll retry in case
                                 // the system's cached authentication token had already expired.
                                 AccountManager authenticator = AccountManager.get(mUi);
                                 mAlreadyTried = true;
 
                                 Log.w("auth", "Requesting renewal of rejected auth token");
                                 authenticator.invalidateAuthToken(mAccount.type, mToken);
                                 mToken = null;
                                 authenticator.getAuthToken(
                                         mAccount, TOKEN_SCOPE, null, mUi, this, mNetwork);
 
                                 // We're not in an error state *yet*.
                                 return;
                             }
                         }
 
                         // We didn't even get an account, so the auth server is likely unreachable.
                         explanation = getString(R.string.error_bad_connection);
                     } else {
                         // Authentication truly failed.
                         Log.e("auth", "Fresh auth token was also rejected");
                         explanation = getString(R.string.error_auth_failed);
                     }
                 } else if (ex instanceof JSONException) {
                     explanation = getString(R.string.error_unexpected_response);
-                    runOnUiThread(new HostListDisplayer(mUi));
                 }
 
                 mHosts = null;
                 Log.w("auth", ex);
                 Toast.makeText(mUi, explanation, Toast.LENGTH_LONG).show();
             }
 
             // Share our findings with the user.
-            runOnUiThread(new HostListDisplayer(mUi));
+            runOnUiThread(new Runnable() {
+                    @Override
+                    public void run() {
+                        updateUi();
+                    }
+            });
         }
     }
 
-    /** Formats the host list and offers it to the user. */
-    private class HostListDisplayer implements Runnable {
-        /** Communication with the screen. */
-        private Activity mUi;
-
-        /** Constructor. */
-        public HostListDisplayer(Activity ui) {
-            mUi = ui;
+    /**
+     * Updates the infotext and host list display.
+     * This method affects the UI and must be run on the main thread.
+     */
+    private void updateUi() {
+        synchronized (mLock) {
+            mRefreshButton.setEnabled(mAccount != null);
+            if (mAccount != null) {
+                mAccountSwitcher.setTitle(mAccount.name);
+            }
         }
 
-        /**
-         * Updates the infotext and host list display.
-         * This method affects the UI and must be run on its same thread.
-         */
-        @Override
-        public void run() {
-            synchronized (mUi) {
-                mRefreshButton.setEnabled(mAccount != null);
-                if (mAccount != null) {
-                    mAccountSwitcher.setTitle(mAccount.name);
-                }
-            }
-
-            if (mHosts == null) {
-                mGreeting.setText(getString(R.string.inst_empty_list));
-                mList.setAdapter(null);
-                return;
-            }
-
-            mGreeting.setText(getString(R.string.inst_host_list));
-
-            ArrayAdapter<JSONObject> displayer = new HostListAdapter(mUi, R.layout.host);
-            Log.i("hostlist", "About to populate host list display");
-            try {
-                int index = 0;
-                while (!mHosts.isNull(index)) {
-                    displayer.add(mHosts.getJSONObject(index));
-                    ++index;
-                }
-                mList.setAdapter(displayer);
-            }
-            catch(JSONException ex) {
-                Log.w("hostlist", ex);
-                Toast.makeText(
-                        mUi, getString(R.string.error_cataloging_hosts), Toast.LENGTH_LONG).show();
-
-                // Close the application.
-                finish();
-            }
-        }
-    }
-
-    /** Describes the appearance and behavior of each host list entry. */
-    private class HostListAdapter extends ArrayAdapter<JSONObject> {
-        /** Constructor. */
-        public HostListAdapter(Context context, int textViewResourceId) {
-            super(context, textViewResourceId);
+        if (mHosts == null) {
+            mGreeting.setText(getString(R.string.inst_empty_list));
+            mList.setAdapter(null);
+            return;
         }
 
-        /** Generates a View corresponding to this particular host. */
-        @Override
-        public View getView(int position, View convertView, ViewGroup parent) {
-            TextView target = (TextView)super.getView(position, convertView, parent);
+        mGreeting.setText(getString(R.string.inst_host_list));
 
-            try {
-                final JSONObject host = getItem(position);
-                target.setText(Html.fromHtml(host.getString("hostName") + " (<font color = \"" +
-                        (host.getString("status").equals("ONLINE") ? HOST_COLOR_ONLINE :
-                        HOST_COLOR_OFFLINE) + "\">" + host.getString("status") + "</font>)"));
+        ArrayAdapter<JSONObject> displayer = new HostListAdapter(this, R.layout.host);
+        Log.i("hostlist", "About to populate host list display");
+        try {
+            int index = 0;
+            while (!mHosts.isNull(index)) {
+                displayer.add(mHosts.getJSONObject(index));
+                ++index;
+            }
+            mList.setAdapter(displayer);
+        } catch (JSONException ex) {
+            Log.w("hostlist", ex);
+            Toast.makeText(this, getString(R.string.error_cataloging_hosts),
+                    Toast.LENGTH_LONG).show();
 
-                if (host.getString("status").equals("ONLINE")) {  // Host is online.
-                    target.setOnClickListener(new View.OnClickListener() {
-                            @Override
-                            public void onClick(View v) {
-                                try {
-                                    synchronized (getContext()) {
-                                        JniInterface.connectToHost(mAccount.name, mToken,
-                                                host.getString("jabberId"),
-                                                host.getString("hostId"),
-                                                host.getString("publicKey"),
-                                                new Runnable() {
-                                            @Override
-                                            public void run() {
-                                                startActivity(
-                                                        new Intent(getContext(), Desktop.class));
-                                            }
-                                        });
-                                    }
-                                }
-                                catch(JSONException ex) {
-                                    Log.w("host", ex);
-                                    Toast.makeText(getContext(),
-                                            getString(R.string.error_reading_host),
-                                            Toast.LENGTH_LONG).show();
-
-                                    // Close the application.
-                                    finish();
-                                }
-                            }
-                        });
-                } else {  // Host is offline.
-                    // Disallow interaction with this entry.
-                    target.setEnabled(false);
-                }
-            }
-            catch(JSONException ex) {
-                Log.w("hostlist", ex);
-                Toast.makeText(getContext(),
-                        getString(R.string.error_displaying_host),
-                        Toast.LENGTH_LONG).show();
-
-                // Close the application.
-                finish();
-            }
-
-            return target;
+            // Close the application.
+            finish();
         }
     }
 }