Android Chromoting - Close the Desktop view on disconnection

Android Chromoting - Close the Desktop view on disconnection

If the session becomes disconnected, this hides the desktop display
instead of leaving it visible and non-interactive.

BUG=325593
TEST=Disconnect the session by killing the host process.

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

[Lambros, 2013-12-28 00:55:46]

This is based off https://codereview.chromium.org/102273006/

[Sergey Ulanov, 2013-12-28 02:51:49]

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:57: void
onDisconnected();
I think it's not enough to have just these two methods. At very least we want to
pass an error code the the UI layer. I think it's better to replace it with a
single method OnStateChanged(state, error), so that the listener is notified
about all connection state changes.

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:168:
sProgressIndicator = ProgressDialog.show(sContext, sContext.
It's not right that the JNI layer handles UI. I think we want to pass state to
the ConnectionListener and then handle the state there.

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:199:
Toast.makeText(sContext, sContext.getResources().getStringArray(
Same here. JNI layer shouldn't handle UI. Let's move it out of here.

[Lambros, 2013-12-31 00:05:05]

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:57: void
onDisconnected();
On 2013/12/28 02:51:49, Sergey Ulanov wrote:
> I think it's not enough to have just these two methods. At very least we want
to
> pass an error code the the UI layer. I think it's better to replace it with a
> single method OnStateChanged(state, error), so that the listener is notified
> about all connection state changes.

Done.

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:168:
sProgressIndicator = ProgressDialog.show(sContext, sContext.
On 2013/12/28 02:51:49, Sergey Ulanov wrote:
> It's not right that the JNI layer handles UI. I think we want to pass state to
> the ConnectionListener and then handle the state there.

Done.

https://codereview.chromium.org/105943010/diff/1/remoting/android/java/src/or...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:199:
Toast.makeText(sContext, sContext.getResources().getStringArray(
On 2013/12/28 02:51:49, Sergey Ulanov wrote:
> Same here. JNI layer shouldn't handle UI. Let's move it out of here.

Done.

[Sergey Ulanov, 2014-01-03 00:01:50]

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:365: if (state
< SUCCESSFUL_CONNECTION && error == 0) {
error is meaningful only when state==FAILED, so checking error==0 doesn't make
sense here. I suggest that you add "switch (state)" here instead of these ifs.

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:395: + (error
== 0 ? "" : ": "
no need to check error==0 here. error shouldn't be 0 when connection failed.

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:48: *
stages of establishing a connection. States greater than this indicate
disconnection
Why not declare constant for all states, not only these two? Then you wouldn't
need to compare values.

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:63: void
onConnectionState(int state, int error);
Also please copy enum values for the errors from remoting/protocol/errors.h .
Second argument is not useful without it.

[Lambros, 2014-01-04 01:59:46]

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:365: if (state
< SUCCESSFUL_CONNECTION && error == 0) {
On 2014/01/03 00:01:51, Sergey Ulanov wrote:
> error is meaningful only when state==FAILED, so checking error==0 doesn't make
> sense here. I suggest that you add "switch (state)" here instead of these ifs.

Done.

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:395: + (error
== 0 ? "" : ": "
On 2014/01/03 00:01:51, Sergey Ulanov wrote:
> no need to check error==0 here. error shouldn't be 0 when connection failed.

Done.

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:48: *
stages of establishing a connection. States greater than this indicate
disconnection
On 2014/01/03 00:01:51, Sergey Ulanov wrote:
> Why not declare constant for all states, not only these two? Then you wouldn't
> need to compare values.

Done.

https://codereview.chromium.org/105943010/diff/60001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:63: void
onConnectionState(int state, int error);
On 2014/01/03 00:01:51, Sergey Ulanov wrote:
> Also please copy enum values for the errors from remoting/protocol/errors.h .
> Second argument is not useful without it.

Second argument is used in an array lookup, loaded from XML resources.
But I've added these constants anyway, and cleaned up their names to use common
prefixes.

[Sergey Ulanov, 2014-01-08 20:30:45]

lgtm, I have couple of comments, but they don't need to be addressed in this CL.

https://codereview.chromium.org/105943010/diff/180001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/105943010/diff/180001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:399:
Toast.makeText(this, stateText + ": "
Appending localized string is wrong - it may not work for some languages. Not a
problem right now because Android is not localized yet.

https://codereview.chromium.org/105943010/diff/180001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/105943010/diff/180001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:49: int
STATE_INITIALIZING = 0;
Can we use Java enums here? In the past it was recommended to avoid enums in
Android code because Dalvik wasn't good at handling them. AFAICT it's not the
case anymore. Neither perf tips nor style guide mention it (see
http://developer.android.com/training/articles/perf-tips.html and
http://source.android.com/source/code-style.html).

[Lambros, 2014-01-08 22:30:01]

FYI

https://codereview.chromium.org/105943010/diff/180001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java
(right):

https://codereview.chromium.org/105943010/diff/180001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:49: int
STATE_INITIALIZING = 0;
On 2014/01/08 20:30:45, Sergey Ulanov wrote:
> Can we use Java enums here? In the past it was recommended to avoid enums in
> Android code because Dalvik wasn't good at handling them. AFAICT it's not the
> case anymore. Neither perf tips nor style guide mention it (see
> http://developer.android.com/training/articles/perf-tips.html and
> http://source.android.com/source/code-style.html).

Done.

[commit-bot: I haz the power, 2014-01-08 22:46:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/105943010/...

[commit-bot: I haz the power, 2014-01-09 00:12:21]

Retried try job too often on linux_aura for step(s) app_list_unittests,
aura_unittests, browser_tests, compositor_unittests, content_browsertests,
content_unittests, events_unittests, interactive_ui_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2014-01-09 00:41:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/105943010/...

[commit-bot: I haz the power, 2014-01-09 07:43:41]

Change committed as 243800

[frankf, 2014-01-10 18:31:05]

https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:415: }
fidnbugs step on FYI bot is complaining about default case missing here:

http://build.chromium.org/p/chromium.fyi/builders/Android%20Builder%20%28dbg%...

[Lambros, 2014-01-10 18:41:00]

On 2014/01/10 18:31:05, frankf wrote:
>
https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
> File remoting/android/java/src/org/chromium/chromoting/Chromoting.java
(right):
> 
>
https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
> remoting/android/java/src/org/chromium/chromoting/Chromoting.java:415: }
> fidnbugs step on FYI bot is complaining about default case missing here:
> 
>
http://build.chromium.org/p/chromium.fyi/builders/Android%2520Builder%2520%25...

Thanks Frank! I must have forgotten to run findbugs this time.

If I'm not mistaken, this switch-case covers every possible state of the Java
enum, so adding a "default" case is both unnecessary and wrong IMO. Is findbugs
not smart enough to deal with this?

Guess I'll have to grit my teeth and add a "default"?

[frankf, 2014-01-10 18:47:00]

On 2014/01/10 18:41:00, Lambros wrote:
> On 2014/01/10 18:31:05, frankf wrote:
> >
>
https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
> > File remoting/android/java/src/org/chromium/chromoting/Chromoting.java
> (right):
> > 
> >
>
https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
> > remoting/android/java/src/org/chromium/chromoting/Chromoting.java:415: }
> > fidnbugs step on FYI bot is complaining about default case missing here:
> > 
> >
>
http://build.chromium.org/p/chromium.fyi/builders/Android%252520Builder%25252...
> 
> Thanks Frank! I must have forgotten to run findbugs this time.
> 
> If I'm not mistaken, this switch-case covers every possible state of the Java
> enum, so adding a "default" case is both unnecessary and wrong IMO. Is
findbugs
> not smart enough to deal with this?
> 
> Guess I'll have to grit my teeth and add a "default"?

Yes, google style guide requires it (No mention in Android style). You might add
another
enum in the future so it's safer to add the default. Looking at several
instances,
it's also done everywhere in the code base.

[phoglund_chromium, 2014-01-13 16:19:48]

https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/105943010/diff/260001/remoting/android/java/s...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:415: }
On 2014/01/10 18:31:05, frankf wrote:
> fidnbugs step on FYI bot is complaining about default case missing here:
> 
>
http://build.chromium.org/p/chromium.fyi/builders/Android%2520Builder%2520%25...

+1: please fix.