Separate singleton out of ChromotingJNIInstance

Separate singleton out of ChromotingJNIInstance

Platform prerequisite and intersession components of this class have been culled into the new ChromotingJNI singleton, which holds an instance of ChromotingJNIInstance during each connection.  This paves the way for future concurrent sessions support.

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

[garykac, 2013-07-15 21:13:42]

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni.cc (right):

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.cc:66: const char* auth_token,
align args

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.cc:90: JNIEnv* env =
base::android::AttachCurrentThread();
DCHECK ui thread?

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni.h (right):

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.h:53: const char* auth_token,
align args

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.h:80: // DisconnectFromHost() before this
singleton is destroyed. Because
Chromium DisconnectFromHost?

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni_instance.h:45: // up. Must be called before
destruction. Call from the UI thread.
"Call from the UI thread" is not needed because the code switches to the UI
thread if needed.

[solb, 2013-07-15 21:49:42]

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni.cc (right):

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.cc:66: const char* auth_token,
On 2013/07/15 21:13:42, garykac wrote:
> align args

Done.

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.cc:90: JNIEnv* env =
base::android::AttachCurrentThread();
On 2013/07/15 21:13:42, garykac wrote:
> DCHECK ui thread?

Done.

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni.h (right):

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.h:53: const char* auth_token,
On 2013/07/15 21:13:42, garykac wrote:
> align args

Done.

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni.h:80: // DisconnectFromHost() before this
singleton is destroyed. Because
On 2013/07/15 21:13:42, garykac wrote:
> Chromium DisconnectFromHost?

Find-and-replace fail. Thanks for finding it!

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19253003/diff/7001/remoting/client/jni/chromo...
remoting/client/jni/chromoting_jni_instance.h:45: // up. Must be called before
destruction. Call from the UI thread.
On 2013/07/15 21:13:42, garykac wrote:
> "Call from the UI thread" is not needed because the code switches to the UI
> thread if needed.

Done.

[garykac, 2013-07-15 22:04:59]

lgtm

[commit-bot: I haz the power, 2013-07-15 22:06:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19253003/15001

[commit-bot: I haz the power, 2013-07-16 03:42:15]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 05:23:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19253003/15001

[commit-bot: I haz the power, 2013-07-16 06:19:42]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-07-16 07:43:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19253003/15001

[commit-bot: I haz the power, 2013-07-16 09:20:22]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Wez, 2013-07-16 13:46:55]

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni.cc (right):

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.cc:22: JNIEnv* env =
base::android::AttachCurrentThread();
nit: Add a comment to explain that this is required before making any calls into
Java on the thread.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.cc:25: // on Android. An AtExitManager cleans
this up at world's end.
nit: An -> The
nit: world's end -> process exit

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni.h (right):

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:21: const char* const JAVA_CLASS =
"org/chromium/chromoting/jni/JNIInterface";
Does this need to be in the header? Where else is it used?

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:65: DCHECK(session_);
This will check if no connection attempt has been initiated; either remove the
DCHECK or update the comment.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:91: scoped_ptr<base::AtExitManager>
collector_;
nit: at_exit_manager_

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:96: // Runners that allow posting tasks to
the various native threads.
nit: Suggest: "References to native threads."

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:104: scoped_refptr<ChromotingJniInstance>
session_;
nit: session_ -> instance_

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.h:34: public
base::RefCountedThreadSafe<ChromotingJniInstance> {
Why does this need to be RefCountedThreadSafe?

[solb, 2013-07-16 19:23:02]

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni.cc (right):

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.cc:22: JNIEnv* env =
base::android::AttachCurrentThread();
On 2013/07/16 13:46:56, Wez wrote:
> nit: Add a comment to explain that this is required before making any calls
into
> Java on the thread.

Done.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.cc:25: // on Android. An AtExitManager cleans
this up at world's end.
On 2013/07/16 13:46:56, Wez wrote:
> nit: An -> The
> nit: world's end -> process exit

Done.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni.h (right):

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:21: const char* const JAVA_CLASS =
"org/chromium/chromoting/jni/JNIInterface";
On 2013/07/16 13:46:56, Wez wrote:
> Does this need to be in the header? Where else is it used?

Done.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:65: DCHECK(session_);
On 2013/07/16 13:46:56, Wez wrote:
> This will check if no connection attempt has been initiated; either remove the
> DCHECK or update the comment.

Done.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:91: scoped_ptr<base::AtExitManager>
collector_;
On 2013/07/16 13:46:56, Wez wrote:
> nit: at_exit_manager_

Done.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:96: // Runners that allow posting tasks to
the various native threads.
On 2013/07/16 13:46:56, Wez wrote:
> nit: Suggest: "References to native threads."

Done.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni.h:104: scoped_refptr<ChromotingJniInstance>
session_;
On 2013/07/16 13:46:56, Wez wrote:
> nit: session_ -> instance_

I believe this would make things less clear; |instance_| could be any object,
whereas |session_| makes it clearer that this represents a specific connection.

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/19253003/diff/15001/remoting/client/jni/chrom...
remoting/client/jni/chromoting_jni_instance.h:34: public
base::RefCountedThreadSafe<ChromotingJniInstance> {
On 2013/07/16 13:46:56, Wez wrote:
> Why does this need to be RefCountedThreadSafe?

In order to maintain thread safety in ChromotingJni. That class needs to create
an instance of this one on the UI thread; however, this one must be torn down
almost entirely on the network thread. If this weren't ref counted, there would
be a weird period while an instance's members were being destroyed but
ChromotingJni still held a reference to the (now terribly invalid) object. The
way it is now, the UI thread can post the cleanup procedure to the network
thread and immediately nullify the pointer to the class; thus, the singleton can
forget about the instance as soon as it becomes invalid.

[commit-bot: I haz the power, 2013-07-16 19:32:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19253003/9002

[commit-bot: I haz the power, 2013-07-17 02:37:50]

Retried try job too often on win7_aura for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-07-17 04:08:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/solb@chromium.org/19253003/9002

[commit-bot: I haz the power, 2013-07-17 05:57:55]

Change committed as 211965