Replace LooperExecutor with built-in class in Android AppRTC Demo

Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.

Committed: https://crrev.com/7cf11476b956145f2dea7252eacdf02c7b8f1a7b
Cr-Commit-Position: refs/heads/master@{#12823}

[sakal, 2016-05-19 11:45:51]

Description was changed from

==========
Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.
==========

to

==========
Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.
==========

[sakal, 2016-05-19 11:45:52]

sakal@webrtc.org changed reviewers:
+ magjed@webrtc.org

[sakal, 2016-05-19 11:46:07]

Magnus, please take a look.

[magjed_webrtc, 2016-05-19 12:07:55]

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (left):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:318:
cpuMonitor.release();
don't we need to shut down the ExecutorService?

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:32: import
org.appspot.apprtc.util.LooperExecutor;
Move this import back to the other org.appspot.apprtc imports.

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:41: import
java.util.concurrent.Executors;
not used?

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java:151:
executor.shutdown();
What happens if connectToRoom() is called again? Better to shut down in
onTCPClose/onTCPError?

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java:39:
private AppRTCUtils.NonThreadSafe executorThreadCheck;
Can you use ThreadUtils.ThreadChecker in
webrtc/base/java/src/org/webrtc/ThreadUtils.java instead? (And nuke
AppRTCUtils.NonThreadSafe in a separate CL).

Also, make thread checker final and initialize here or in the ctor, and call
detachThread() in the ctor. Then remove checkIfCalledOnValidThread() and inline
all uses with executorThreadCheck.calledOnValidThread().

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidjunit/...
File
webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidjunit/...
webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java:213:
public void executeAndWait(Runnable runnable) {
Can you implement it like this?
public void executeAndWait(Runnable runnable) {
  executor.submit(runnable).get();
}

[sakal, 2016-05-19 12:39:03]

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java (left):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:318:
cpuMonitor.release();
On 2016/05/19 12:07:54, magjed_webrtc wrote:
> don't we need to shut down the ExecutorService?

In the new implementation, the executor is already destroyed on onPause.

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:32: import
org.appspot.apprtc.util.LooperExecutor;
On 2016/05/19 12:07:54, magjed_webrtc wrote:
> Move this import back to the other org.appspot.apprtc imports.

Done.

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/CallActivity.java:41: import
java.util.concurrent.Executors;
On 2016/05/19 12:07:54, magjed_webrtc wrote:
> not used?

Done.

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/DirectRTCClient.java:151:
executor.shutdown();
On 2016/05/19 12:07:54, magjed_webrtc wrote:
> What happens if connectToRoom() is called again? Better to shut down in
> onTCPClose/onTCPError?

Executor gets shutdown too early if it is shutdown on onTCPClose. AppRTCClient
objects are currently treated as single use objects. If we want to make the code
future proof, we can remove final from Executor and reinitialize it on
connectToRoom.

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
File webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidapp/sr...
webrtc/examples/androidapp/src/org/appspot/apprtc/TCPChannelClient.java:39:
private AppRTCUtils.NonThreadSafe executorThreadCheck;
On 2016/05/19 12:07:54, magjed_webrtc wrote:
> Can you use ThreadUtils.ThreadChecker in
> webrtc/base/java/src/org/webrtc/ThreadUtils.java instead? (And nuke
> AppRTCUtils.NonThreadSafe in a separate CL).
> 
> Also, make thread checker final and initialize here or in the ctor, and call
> detachThread() in the ctor. Then remove checkIfCalledOnValidThread() and
inline
> all uses with executorThreadCheck.calledOnValidThread().

Done.

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidjunit/...
File
webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java
(right):

https://codereview.webrtc.org/1992213002/diff/1/webrtc/examples/androidjunit/...
webrtc/examples/androidjunit/src/org/appspot/apprtc/TCPChannelClientTest.java:213:
public void executeAndWait(Runnable runnable) {
On 2016/05/19 12:07:54, magjed_webrtc wrote:
> Can you implement it like this?
> public void executeAndWait(Runnable runnable) {
>   executor.submit(runnable).get();
> }

Done.

[magjed_webrtc, 2016-05-20 07:43:20]

lgtm

[sakal, 2016-05-20 07:44:52]

The CQ bit was checked by sakal@webrtc.org

[commit-bot: I haz the power, 2016-05-20 07:45:01]

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

[commit-bot: I haz the power, 2016-05-20 09:41:05]

Description was changed from

==========
Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.
==========

to

==========
Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.
==========

[commit-bot: I haz the power, 2016-05-20 09:41:06]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-20 09:41:13]

Description was changed from

==========
Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.
==========

to

==========
Replace LooperExecutor with built-in class in Android AppRTC Demo

LooperExecutor is only truly needed in WebSocketChannelClient because of
WebSocketClient from autobanh requiring thread to have a looper. So
LooperExecutor was left there but replaced everywhere else with built-in
singleThreadExecutor/singleThreadScheduledExecutor.

Motivation behind this change is that built-in class behaves better
under testing environment and doesn't require hacky
RobolectricLooperExecutor.

Committed: https://crrev.com/7cf11476b956145f2dea7252eacdf02c7b8f1a7b
Cr-Commit-Position: refs/heads/master@{#12823}
==========

[commit-bot: I haz the power, 2016-05-20 09:41:14]

Patchset 2 (id:??) landed as
https://crrev.com/7cf11476b956145f2dea7252eacdf02c7b8f1a7b
Cr-Commit-Position: refs/heads/master@{#12823}