[Android Client] Break down multi-threaded classes by thread

ChromotingJniInstance has been a ref-counted class running on multiple threads, which has obscured deletion behavior. This CL drops its ref-counted support and pulls non-network-thread logics out of the class.

Doc: https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc_Q/edit?usp=sharing

BUG=614242
Committed: https://crrev.com/b98e6c2bf39155864da71b8b69470ebc58913719
Cr-Commit-Position: refs/heads/master@{#397456}

[Yuwei, 2016-05-24 19:49:27]

Patchset #1 (id:1) has been deleted

[Yuwei, 2016-05-24 19:49:37]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-24 19:50:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007123003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007123003/20001

[Yuwei, 2016-05-24 21:36:22]

Patchset #2 (id:40001) has been deleted

[Yuwei, 2016-05-24 21:43:12]

Patchset #2 (id:60001) has been deleted

[Yuwei, 2016-05-24 21:47:30]

Description was changed from

==========
Make ChromotingJniInstance single-threaded

BUG=614242
==========

to

==========
Make ChromotingJniInstance single-threaded

ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

BUG=614242
==========

[Yuwei, 2016-05-24 21:47:57]

yuweih@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Yuwei, 2016-05-24 21:47:57]

ptal

[Yuwei, 2016-05-27 03:32:57]

Description was changed from

==========
Make ChromotingJniInstance single-threaded

ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

BUG=614242
==========

to

==========
WIP

ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242
==========

[Yuwei, 2016-05-27 03:32:57]

yuweih@chromium.org changed reviewers:
- lambroslambrou@chromium.org

[Yuwei, 2016-05-27 03:36:19]

Patchset #3 (id:100001) has been deleted

[Yuwei, 2016-05-27 18:40:43]

Patchset #1 (id:20001) has been deleted

[Yuwei, 2016-05-27 18:40:57]

Patchset #1 (id:80001) has been deleted

[Yuwei, 2016-05-27 18:41:14]

Patchset #1 (id:120001) has been deleted

[Yuwei, 2016-05-27 19:12:03]

Description was changed from

==========
WIP

ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242
==========

to

==========
ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

Doc:
https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242
==========

[Yuwei, 2016-05-27 19:12:03]

yuweih@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[Yuwei, 2016-05-27 19:14:13]

This CL is ready to be reviewed (PTAL at the doc if you see anything confusing (
͡° ͜ʖ ͡°) ).

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/Display.java (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Display.java:38: private
final Object mRedrawCallbackLock = new Object();
Mostly moved from Client.java. This lock is additional.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.h:8: #include
<remoting/client/jni/jni_secret_fetcher.h>
Oops... Autogenerated code. Should use double quotes.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.cc (left):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_client.cc:307: DisconnectFromHost();
Moved into the dtor.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:149: void Destroy(JNIEnv* env, const
base::android::JavaParamRef<jobject>& caller);
No longer need env to manually delete the reference in Destroy after using
ScopedJavaGlobalRef.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:157: ChromotingJniRuntime* runtime_;
Feels cleaner and easier to test to pass in the runtime pointer.

[Yuwei, 2016-05-27 20:12:27]

The CQ bit was checked by yuweih@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-27 20:12:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007123003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007123003/140001

[commit-bot: I haz the power, 2016-05-27 21:32:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-27 21:32:58]

Dry run: This issue passed the CQ dry run.

[Yuwei, 2016-05-27 23:15:34]

CC'ed Sergey. Feel free to join if you have any thoughts :)

[Lambros, 2016-05-28 00:43:06]

lgtm

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:32: // Called
on the UI thread.
If everything is called on the UI thread, you can remove all these comments
about UI thread, and instead just have a single comment near the top saying this
class is used on the UI thread.

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:43: *
Better to provide a JavaDoc one-sentence summary, even if it is just:
Returns the display object.

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/Display.java (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Display.java:36: /**
Protects access to {@link mRedrawCallback}. Locking is not required when reading
on the
Format this in multi-line style:
/**
 * Protects...
 * ...blah...
 */

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.cc:512: client_context_.reset();
Should we invalidate weak_factory_ here?

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.h:8: #include
<remoting/client/jni/jni_secret_fetcher.h>
Move this to the lower block of #includes.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.h:41: // network thread.
Give a little more information here about threading. This class is created on
the
UI thread but thereafter it is used and destroyed on the network thread. Except
where indicated, all methods are called on the network thread.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_client.cc:30: // The session must be shut down first,
since it depends on our other
Blank line before comment.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:33: // connection is active (i.e. between a
call to Connect() and the
This sentence looks out-dated; remove it?

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_display_handler.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.cc:19: const int kBytesPerPixel = 4;
Either add blank line here, or remove blank line above.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.cc:36:
base::android::ScopedJavaLocalRef<jobject> JniDisplayHandler::NewBitmap(
Add "// static" comment, and make this a static method.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.cc:38: int height) {
Can fit on previous line. Run git cl format

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_display_handler.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:46: void Destroy(JNIEnv* env,
What thread calls this? Looks like the implementation handles any thread.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:49: void ScheduleRedraw(JNIEnv* env,
Add comment that this is called on the UI thread.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:57:
DISALLOW_COPY_AND_ASSIGN(JniDisplayHandler);
Blank line above DISALLOW...

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:61: #endif  //
REMOTING_CLIENT_JNI_JNI_DISPLAY_HANDLER_H_
Blank line above #endif

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_frame_consumer.cc:69: ->NewBitmap(frame->size().width(),
frame->size().height())
Is this 'git cl format'ed? Seems like too much indent.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_secret_fetcher.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_secret_fetcher.h:17: // This class fetches the secret on
the UI thread. It must be deleted on UI.
pairing secret

[Yuwei, 2016-06-01 21:28:40]

Patchset #4 (id:200001) has been deleted

[Yuwei, 2016-06-01 21:30:11]

I'll send another CL to address comments on the design doc. Thanks for reviewing
this CL!

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/Client.java (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:32: // Called
on the UI thread.
On 2016/05/28 00:43:05, Lambros wrote:
> If everything is called on the UI thread, you can remove all these comments
> about UI thread, and instead just have a single comment near the top saying
this
> class is used on the UI thread.

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Client.java:43: *
On 2016/05/28 00:43:05, Lambros wrote:
> Better to provide a JavaDoc one-sentence summary, even if it is just:
> Returns the display object.

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
File remoting/android/java/src/org/chromium/chromoting/jni/Display.java (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/android/java/...
remoting/android/java/src/org/chromium/chromoting/jni/Display.java:36: /**
Protects access to {@link mRedrawCallback}. Locking is not required when reading
on the
On 2016/05/28 00:43:05, Lambros wrote:
> Format this in multi-line style:
> /**
>  * Protects...
>  * ...blah...
>  */

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_instance.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.cc:512: client_context_.reset();
On 2016/05/28 00:43:05, Lambros wrote:
> Should we invalidate weak_factory_ here? 

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.h:8: #include
<remoting/client/jni/jni_secret_fetcher.h>
On 2016/05/28 00:43:05, Lambros wrote:
> Move this to the lower block of #includes.

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/ch...
remoting/client/jni/chromoting_jni_instance.h:41: // network thread.
On 2016/05/28 00:43:05, Lambros wrote:
> Give a little more information here about threading. This class is created on
> the
> UI thread but thereafter it is used and destroyed on the network thread.
Except
> where indicated, all methods are called on the network thread.
> 

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_client.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_client.h:33: // connection is active (i.e. between a
call to Connect() and the
On 2016/05/28 00:43:05, Lambros wrote:
> This sentence looks out-dated; remove it?

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_display_handler.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.cc:19: const int kBytesPerPixel = 4;
On 2016/05/28 00:43:05, Lambros wrote:
> Either add blank line here, or remove blank line above.

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.cc:36:
base::android::ScopedJavaLocalRef<jobject> JniDisplayHandler::NewBitmap(
On 2016/05/28 00:43:05, Lambros wrote:
> Add "// static" comment, and make this a static method.

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.cc:38: int height) {
On 2016/05/28 00:43:05, Lambros wrote:
> Can fit on previous line. Run git cl format

It doesn't seem like cl format makes any change... The Chromium style guide says
"put each argument on a separate line unless the whole declaration fits on one
line"...

https://www.chromium.org/developers/coding-style

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_display_handler.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:46: void Destroy(JNIEnv* env,
On 2016/05/28 00:43:06, Lambros wrote:
> What thread calls this? Looks like the implementation handles any thread.

Done. Added comments.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:49: void ScheduleRedraw(JNIEnv* env,
On 2016/05/28 00:43:06, Lambros wrote:
> Add comment that this is called on the UI thread.

I just wrote "can be called on any thread"...

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:57:
DISALLOW_COPY_AND_ASSIGN(JniDisplayHandler);
On 2016/05/28 00:43:06, Lambros wrote:
> Blank line above DISALLOW...

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_display_handler.h:61: #endif  //
REMOTING_CLIENT_JNI_JNI_DISPLAY_HANDLER_H_
On 2016/05/28 00:43:05, Lambros wrote:
> Blank line above #endif

Done.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_frame_consumer.cc (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_frame_consumer.cc:69: ->NewBitmap(frame->size().width(),
frame->size().height())
On 2016/05/28 00:43:06, Lambros wrote:
> Is this 'git cl format'ed? Seems like too much indent.

Yep... git cl format actually makes it like this... No idea what style rule
produces this.

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
File remoting/client/jni/jni_secret_fetcher.h (right):

https://codereview.chromium.org/2007123003/diff/140001/remoting/client/jni/jn...
remoting/client/jni/jni_secret_fetcher.h:17: // This class fetches the secret on
the UI thread. It must be deleted on UI.
On 2016/05/28 00:43:06, Lambros wrote:
> pairing secret

Done.

[Yuwei, 2016-06-01 21:30:39]

The CQ bit was checked by yuweih@chromium.org

[Yuwei, 2016-06-01 21:30:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
lambroslambrou@chromium.org
Link to the patchset: https://codereview.chromium.org/2007123003/#ps220001
(title: "Rename JniSecretFetcher to JniPairingSecretFetcher")

[commit-bot: I haz the power, 2016-06-01 21:31:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007123003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007123003/220001

[commit-bot: I haz the power, 2016-06-02 01:59:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 01:59:34]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuwei, 2016-06-02 03:21:35]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2016-06-02 03:21:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007123003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007123003/220001

[commit-bot: I haz the power, 2016-06-02 04:06:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-02 04:06:09]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuwei, 2016-06-02 16:43:31]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2016-06-02 16:44:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2007123003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2007123003/220001

[commit-bot: I haz the power, 2016-06-02 17:29:25]

Description was changed from

==========
ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

Doc:
https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242
==========

to

==========
ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

Doc:
https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242
==========

[commit-bot: I haz the power, 2016-06-02 17:29:26]

Committed patchset #4 (id:220001)

[commit-bot: I haz the power, 2016-06-02 17:30:58]

Description was changed from

==========
ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

Doc:
https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242
==========

to

==========
ChromotingJniInstance has been a ref-counted class running on multiple threads,
which has obscured deletion behavior. This CL drops its ref-counted support and
pulls non-network-thread logics out of the class.

Doc:
https://docs.google.com/document/d/1ZK-UQSC_rTjNsBa_MFbCmCh3fQRPFuVAwKhWnTZqc...

BUG=614242

Committed: https://crrev.com/b98e6c2bf39155864da71b8b69470ebc58913719
Cr-Commit-Position: refs/heads/master@{#397456}
==========

[commit-bot: I haz the power, 2016-06-02 17:30:59]

Patchset 4 (id:??) landed as
https://crrev.com/b98e6c2bf39155864da71b8b69470ebc58913719
Cr-Commit-Position: refs/heads/master@{#397456}