Add the beginnings of a Chromoting Android app

Add the beginnings of a Chromoting Android app

Currently, this has only the following capabilities:
 + Authenticate using a Google account on the phone
 + Query and display the host list from the Chromoting directory server
 + Connect to and communicate with the host service over XMPP/ICE
 + Establish peer-to-peer channels for communicating with the host service
Notable missing features are:
 - Display the host's desktop
 - Handle any kind of input

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

[solb, 2013-07-17 08:36:26]

Here's enough Java code to build an app that actually builds, runs, and is
somewhat useful! This would be applied on top of issue 19297003.

[garykac, 2013-07-17 23:52:28]

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
File remoting/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
remoting/android/java/res/values/strings.xml:8: <string
name="inst_host_list">\'s remote desktop hosts:</string>
This sounds like it's going to be strcat'ed after the username.  If so, this
won't localize well into languages that want to username to be in the middle or
end of the string.

Can you put a placeholder of some sort here?

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
remoting/android/java/res/values/strings.xml:30: <!--protocol states (see
remoting/protocol/connection_to_host.h)-->
If this needs to be kept in sync with connection_to_host.h, please add a comment
there to that effect.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
remoting/android/java/res/values/strings.xml:40: <!--protocol errors (see
remoting/protocol/errors.h)-->
likewise

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:141: * network
operations and must be run an a non-UI thread.
Is there a way to assert this in the code?

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:157: new
URL(HOST_LIST_PATH+JniInterface.getApiKey()).openConnection();
spaces around +

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:210: // Bail
out.
I'm not sure this comment provides more info than the "finish" call.  Does this
exit the dialog or the entire application?

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:232:
mGreeting.setText(mAccount.name+getString(R.string.inst_host_list));
spaces around +

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:270:
(host.getString("status").equals("ONLINE") ? HOST_COLOR_ONLINE :
This looks like it would only work for English.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:284: //
TODO(solb) Start an Activity to display the desktop.
I'm not sure it's worthwhile to add it, but is there a NYI() equivalent you
could use here.

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

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:72: if
(sConnected) {
Tab?

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:103: /**
Screen sWidth of the video feed. */
Screen width

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:106: /**
Screen sHeight of the video feed. */
Screen height

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:119:
sContext.getResources().getStringArray(R.array.protoc_states)[state]+(error!=0 ?
Can this be formatted better?

    sContext.getResources().getStringArray(
        R.array.protoc_states)[state] +
            (error != 0 ? ": " +
sContext.getResources().getStringArray(R.array.protoc_errors)[error]
                        : ""),
        Toast.LENGTH_SHORT).show();

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:131:
pinEntry.setInputType(InputType.TYPE_CLASS_NUMBER|InputType.TYPE_NUMBER_VARIATION_PASSWORD);
spaces around |

[solb, 2013-07-18 01:23:36]

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
File remoting/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
remoting/android/java/res/values/strings.xml:8: <string
name="inst_host_list">\'s remote desktop hosts:</string>
On 2013/07/17 23:52:28, garykac wrote:
> This sounds like it's going to be strcat'ed after the username.  If so, this
> won't localize well into languages that want to username to be in the middle
or
> end of the string.
> 
> Can you put a placeholder of some sort here?

Yes, but I'm instead switching to simple static text for brevity.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
remoting/android/java/res/values/strings.xml:30: <!--protocol states (see
remoting/protocol/connection_to_host.h)-->
On 2013/07/17 23:52:28, garykac wrote:
> If this needs to be kept in sync with connection_to_host.h, please add a
comment
> there to that effect.

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/res/val...
remoting/android/java/res/values/strings.xml:40: <!--protocol errors (see
remoting/protocol/errors.h)-->
On 2013/07/17 23:52:28, garykac wrote:
> likewise

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
File remoting/android/java/src/org/chromium/chromoting/Chromoting.java (right):

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:141: * network
operations and must be run an a non-UI thread.
On 2013/07/17 23:52:28, garykac wrote:
> Is there a way to assert this in the code?

Everything I've seen indicates that actual assertions are disabled by default on
the Dalvik VM, and that enabling them requires execution of a shell command on
the Android device. This doesn't seem worth the effort, since we'd have to
manually switch on such assertions prior to running the app to find failures. I
could use JUnit assertions, but they're really only intended for unit testing,
and I think convincing GYP to exclude them from release builds might be
challenging. I could just print a log message, or use Log.wtf() from
http://developer.android.com/reference/android/util/Log.html. Which approach do
you prefer?

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:157: new
URL(HOST_LIST_PATH+JniInterface.getApiKey()).openConnection();
On 2013/07/17 23:52:28, garykac wrote:
> spaces around +

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:210: // Bail
out.
On 2013/07/17 23:52:28, garykac wrote:
> I'm not sure this comment provides more info than the "finish" call.  Does
this
> exit the dialog or the entire application?

Good point: I'm rewording this to, "Close the application."

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:232:
mGreeting.setText(mAccount.name+getString(R.string.inst_host_list));
On 2013/07/17 23:52:28, garykac wrote:
> spaces around +

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:270:
(host.getString("status").equals("ONLINE") ? HOST_COLOR_ONLINE :
On 2013/07/17 23:52:28, garykac wrote:
> This looks like it would only work for English.

Okay if I fix this in a separate internationalization CL?

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/Chromoting.java:284: //
TODO(solb) Start an Activity to display the desktop.
On 2013/07/17 23:52:28, garykac wrote:
> I'm not sure it's worthwhile to add it, but is there a NYI() equivalent you
> could use here.

I'm afraid I don't know of one. This is a placeholder for stuff in an upcoming
CL, though, so hopefully it won't be here for long.

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

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:72: if
(sConnected) {
On 2013/07/17 23:52:28, garykac wrote:
> Tab?

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:103: /**
Screen sWidth of the video feed. */
On 2013/07/17 23:52:28, garykac wrote:
> Screen width

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:106: /**
Screen sHeight of the video feed. */
On 2013/07/17 23:52:28, garykac wrote:
> Screen height

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:119:
sContext.getResources().getStringArray(R.array.protoc_states)[state]+(error!=0 ?
On 2013/07/17 23:52:28, garykac wrote:
> Can this be formatted better?
> 
>     sContext.getResources().getStringArray(
>         R.array.protoc_states)[state] +
>             (error != 0 ? ": " +
> sContext.getResources().getStringArray(R.array.protoc_errors)[error]
>                         : ""),
>         Toast.LENGTH_SHORT).show();

Done.

https://codereview.chromium.org/19506004/diff/1/remoting/android/java/src/org...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:131:
pinEntry.setInputType(InputType.TYPE_CLASS_NUMBER|InputType.TYPE_NUMBER_VARIATION_PASSWORD);
On 2013/07/17 23:52:28, garykac wrote:
> spaces around |

Done.

[Wez, 2013-07-18 17:08:42]

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/Andr...
File remoting/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/Andr...
remoting/android/java/AndroidManifest.xml:7: android:targetSdkVersion="14"/>
nit: Indentation.

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/Andr...
remoting/android/java/AndroidManifest.xml:13:
android:icon="@drawable/chromoting128">
nit: Indentation.

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/res/...
File remoting/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/res/...
remoting/android/java/res/layout/main.xml:7:
android:layout_height="wrap_content"
nit: Indentation.

[solb, 2013-07-18 17:47:49]

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/Andr...
File remoting/android/java/AndroidManifest.xml (right):

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/Andr...
remoting/android/java/AndroidManifest.xml:7: android:targetSdkVersion="14"/>
On 2013/07/18 17:08:42, Wez wrote:
> nit: Indentation.

Done.

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/Andr...
remoting/android/java/AndroidManifest.xml:13:
android:icon="@drawable/chromoting128">
On 2013/07/18 17:08:42, Wez wrote:
> nit: Indentation.

Done.

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/res/...
File remoting/android/java/res/layout/main.xml (right):

https://codereview.chromium.org/19506004/diff/8001/remoting/android/java/res/...
remoting/android/java/res/layout/main.xml:7:
android:layout_height="wrap_content"
On 2013/07/18 17:08:42, Wez wrote:
> nit: Indentation.

Done.

[solb, 2013-07-18 20:29:57]

Hopefully the bots don't refuse to apply *this* patch. Does it look ready to go
to CQ?

[garykac, 2013-07-18 20:33:26]

lgtm

[commit-bot: I haz the power, 2013-07-18 21:56:51]

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

[commit-bot: I haz the power, 2013-07-19 01:49:10]

Change committed as 212496