Restructure chromoting_jni_instance handling of Java strings

remoting/client/jni/chromoting_jni_instance.h

Index: remoting/client/jni/chromoting_jni_instance.h
diff --git a/remoting/client/jni/chromoting_jni_instance.h b/remoting/client/jni/chromoting_jni_instance.h
index 75ef56bd87f10c12073bd3a77202e69422bc9ef9..ae53b84f014c4738a1a65577ce3d1d8bbe4f6ba2 100644
--- a/remoting/client/jni/chromoting_jni_instance.h
+++ b/remoting/client/jni/chromoting_jni_instance.h
@@ -37,30 +37,32 @@ const bool CHAT_USE_TLS = true;
 const char* const CHAT_AUTH_METHOD = "oauth2";
 
 // ClientUserInterface that makes and (indirectly) receives JNI calls.
+// It also keeps the references to various Chromium components (e.g. message
+// loops and task runners) that must outlive any use of the network stack.

[Wez, 2013/07/11 22:32:08]

Why is the ClientUserInterface maintaining message loops needed by network code?

[solb, 2013/07/11 23:59:09]

Done.

 class ChromotingJNIInstance : public ClientUserInterface {
  public:
+  // This class is instantiated at process initialization and persists until
+  // we close. It reuses many of its components between connections (i.e. when
+  // a DisconnectFromHost() call is followed by a ConnectToHost() one.

[Wez, 2013/07/11 22:32:08]

See style guide re comment style: e.g. Returns the singleton JNI interface.
Re-used across connections.

Why is this re-used across connections?

[solb, 2013/07/11 23:59:09]

This will be addressed in a separate CL that separates out the
ClientUserInterface components.

[Wez, 2013/07/12 00:24:48]

OK, but please address the comment style issue.

   static ChromotingJNIInstance* GetInstance();
 
-  // Call from UI thread.
   void ConnectToHost(

[Wez, 2013/07/11 22:32:08]

Add comments to explain the semantics of these three methods, e.g. what happens
if I can this twice w/out disconnect, etc.

[solb, 2013/07/11 23:59:09]

Done.

[Wez, 2013/07/12 00:24:48]

Chromium comments on methods are things like:
"Starts a connection to |host_jid|, using the |username| and |auth_token| to
authentication to the Talk network. This object can be re-used for multiple
connections by calling DisconnectFromHost() in-between."

-      jstring username,
-      jstring auth_token,
-      jstring host_jid,
-      jstring host_id,
-      jstring host_pubkey);
+      const char* username,
+      const char* auth_token,
+      const char* host_jid,
+      const char* host_id,
+      const char* host_pubkey);
 
-  // Call from UI thread.
   void DisconnectFromHost();
 
-  // Call from UI thread.
-  void AuthenticateWithPin(jstring pin);
+  void AuthenticateWithPin(const char* pin);
 
   // Called by client authenticator.
   // Gets notified if the user needs to enter a PIN, and notifies Java in turn.

[Wez, 2013/07/11 22:32:08]

Suggest: Calls to Java to prompt the user for an authentication PIN.

[solb, 2013/07/11 23:59:09]

Done.

   void FetchSecret(bool pairable,
-                   const protocol::SecretFetchedCallback& callback_encore);
+                   const protocol::SecretFetchedCallback& callback);
 
-  // ClientUserInterface implementation:
+  // ClientUserInterface implementation.
   virtual void OnConnectionState(
       protocol::ConnectionToHost::State state,
       protocol::ErrorCode error) OVERRIDE;
@@ -80,19 +82,25 @@ class ChromotingJNIInstance : public ClientUserInterface {
   void ConnectToHostOnDisplayThread();
   void ConnectToHostOnNetworkThread();
 
-  void DisconnectFromHostOnNetworkThread();
+  // The below variables are reused across consecutive sessions.
+
+  // Reference to the Java class into which we make JNI calls.
+  jclass class_;
 
-  // Reusable between sessions:
-  jclass class_;  // Reference to the Java class into which we make JNI calls.
   scoped_ptr<base::AtExitManager> collector_;

[Wez, 2013/07/11 22:32:08]

nit: Add a comment to explain this.

Does it really need to be a scoped_ptr<>?

[solb, 2013/07/11 23:59:09]

Done. It's a scoped_ptr to avoid keeping a bare pointer that might get leaked.

[Wez, 2013/07/12 00:24:48]

Why is it not just a member, since it has the same lifetime as the containing
object?

+
+  // Chromium code's connection to the Java message loop.
   scoped_ptr<base::MessageLoopForUI> ui_loop_;
-  scoped_refptr<AutoThreadTaskRunner> ui_runner_;
-  scoped_refptr<AutoThreadTaskRunner> net_runner_;
-  scoped_refptr<AutoThreadTaskRunner> disp_runner_;
+
+  // Runners that allow posting tasks to the various native threads.
+  scoped_refptr<AutoThreadTaskRunner> ui_task_runner_;
+  scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
+  scoped_refptr<AutoThreadTaskRunner> display_task_runner_;
+
   scoped_refptr<net::URLRequestContextGetter> url_requester_;
   scoped_refptr<FrameConsumerProxy> frames_;

[Wez, 2013/07/11 22:32:08]

frame_consumer_

[solb, 2013/07/11 23:59:09]

Done.

 
-  // Specific to each session:
+  // The below variables are specific to each connection.

[Wez, 2013/07/11 22:32:08]

Why are per-session variables mixed in with global ones?

[solb, 2013/07/11 23:59:09]

This is for that other CL I referenced above.

   scoped_ptr<ClientConfig> client_config_;
   scoped_ptr<ClientContext> client_context_;
   scoped_ptr<protocol::ConnectionToHost> connection_;
@@ -102,21 +110,13 @@ class ChromotingJNIInstance : public ClientUserInterface {
   scoped_ptr<NetworkSettings> netset_;

[Wez, 2013/07/11 22:32:08]

network_settings_

[solb, 2013/07/11 23:59:09]

Done.

   protocol::SecretFetchedCallback announce_secret_;

[Wez, 2013/07/11 22:32:08]

Why is this called |announce_secret_|?

[solb, 2013/07/11 23:59:09]

Done.

 
-  // Java string handles:
-  jstring username_jstr_;
-  jstring auth_token_jstr_;
-  jstring host_jid_jstr_;
-  jstring host_id_jstr_;
-  jstring host_pubkey_jstr_;
-  jstring pin_jstr_;
-
-  // C string pointers:
-  const char* username_cstr_;
-  const char* auth_token_cstr_;
-  const char* host_jid_cstr_;
-  const char* host_id_cstr_;
-  const char* host_pubkey_cstr_;
-  const char* pin_cstr_;
+  // These strings describe the current connection, and are not reused.
+  std::string username_;
+  std::string auth_token_;
+  std::string host_jid_;
+  std::string host_id_;
+  std::string host_pubkey_;
+  std::string pin_;
 
   friend struct DefaultSingletonTraits<ChromotingJNIInstance>;