Capabilities + Extensions + Cast Host Extension Support for Android client

Capabilities + Extensions + Cast Support for Android client

Support for:
1. Capability enumeration, negotiation and managment.
2. Generic client-side extensions, similar to HostExtension model in Chromoting host.
3. Interaction with Chromoting host's CastExtension and support for Cast Sender API 
(prototype for Cast support).

Note:
The android app has four new dependencies: v4 support, v7 appcompat, v7 mediarouter,
google play services. The first three are in third_party/android_tools/,
but the last isn't. See crbug.com/403948. This CL will not break any builds 
since the code is behind a gyp flag in remoting.gyp.

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

[aiguha, 2014-08-08 05:13:51]

PTAL

https://codereview.chromium.org/451973002/diff/1/remoting/android/.gitignore
File remoting/android/.gitignore (right):

https://codereview.chromium.org/451973002/diff/1/remoting/android/.gitignore#...
remoting/android/.gitignore:1: /google-play-services_lib
Temporarily ignored my local version of google-play-services so it wouldn't get
uploaded.

https://codereview.chromium.org/451973002/diff/1/remoting/android/cast/Androi...
File remoting/android/cast/AndroidManifest.xml.jinja2 (right):

https://codereview.chromium.org/451973002/diff/1/remoting/android/cast/Androi...
remoting/android/cast/AndroidManifest.xml.jinja2:41:
android:theme="@style/Theme.AppCompat"/>
Desktop.java is now an ActionBarActivity part of the v7 appcompat library. I had
to change the theme to make things compatible.

https://codereview.chromium.org/451973002/diff/1/remoting/android/cast/src/or...
File remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java
(right):

https://codereview.chromium.org/451973002/diff/1/remoting/android/cast/src/or...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:155:
sContext, "Failed to connect to Cast device.",
This (and some others like it) looks awkward, but I couldn't find a convention
on how to break up long lines of code like this. Any advice? (Is there a script
I'm overlooking?)

https://codereview.chromium.org/451973002/diff/1/remoting/android/java/res/me...
File remoting/android/java/res/menu/desktop_actionbar.xml (right):

https://codereview.chromium.org/451973002/diff/1/remoting/android/java/res/me...
remoting/android/java/res/menu/desktop_actionbar.xml:13:
app:actionProviderClass="android.support.v7.app.MediaRouteActionProvider"
Cast button.

https://codereview.chromium.org/451973002/diff/1/remoting/remoting_android.gypi
File remoting/remoting_android.gypi (right):

https://codereview.chromium.org/451973002/diff/1/remoting/remoting_android.gy...
remoting/remoting_android.gypi:62: 'android/cast/AndroidManifest.xml.jinja2'
We need to use different manifest files as the new one requires us to specify
google_play_services_version as part of the manifest.

[Lambros, 2014-08-08 23:20:46]

Just a style nit, but a lot of your line-continuations are indented 4-space (or
not at all), where they should be indented 8 spaces.

https://codereview.chromium.org/451973002/diff/20001/remoting/android/cast/sr...
File remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java
(right):

https://codereview.chromium.org/451973002/diff/20001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:150:
Log.e(TAG, "Exception while creating channel.", e);
Yeah, I think this is too deeply nested. It's an anonymous inner class of
another inner class! Can this be flattened somehow?

https://codereview.chromium.org/451973002/diff/20001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:188:
class ChromotocastChannel implements Cast.MessageReceivedCallback {
private?

https://codereview.chromium.org/451973002/diff/20001/remoting/android/java/re...
File remoting/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/451973002/diff/20001/remoting/android/java/re...
remoting/android/java/res/values/strings.xml:4: <string
name="media_route_menu_title">Cast</string>
We shouldn't need a strings.xml file here. We should add the localizable string
to remoting/resources/remoting_strings.grd, and then the GRIT tool generates
localized strings.xml files for us as part of the build (see the
"remoting_strings" target).

I wonder if this string can be pulled from the same library that holds the Cast
logo image?

[aiguha, 2014-08-14 18:35:05]

PTAL. I've done a findbugs pass and made the fixes. Also flattened things out
like you asked and fixed all style nits.
Please tell me if you'd like to do another in-person review.

https://codereview.chromium.org/451973002/diff/20001/remoting/android/cast/sr...
File remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java
(right):

https://codereview.chromium.org/451973002/diff/20001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:150:
Log.e(TAG, "Exception while creating channel.", e);
On 2014/08/08 23:20:45, Lambros wrote:
> Yeah, I think this is too deeply nested. It's an anonymous inner class of
> another inner class! Can this be flattened somehow?

Done.

https://codereview.chromium.org/451973002/diff/20001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:188:
class ChromotocastChannel implements Cast.MessageReceivedCallback {
On 2014/08/08 23:20:45, Lambros wrote:
> private?

Done.

[Lambros, 2014-08-14 23:54:26]

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
File remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:55:
private static final String CHROMOTOCAST_NAMESPACE =
"urn:x-cast:com.chromoting.cast.all";
This should use "com.google.chromeremotedesktop" for official builds. You can
add a TODO for this - don't need to fix it right now.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:141:
showToast("Failed to connect.", Toast.LENGTH_SHORT);
Please localize any toasts. Maybe have a look through
remoting/resources/remoting_strings.grd to see if there's already a generic
"connection failed" message?

Also, do you need to call tearDown() here?

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:220:
public void onVolumeChanged() {} // Changes in volume do not affect us.
nit: two spaces before //

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:263:
//
nit: blank line above this comment block

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:311:

nit: blank line not needed

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:330:
showToast("Closing connection to Cast device.", Toast.LENGTH_SHORT);
Do we need this toast? If so, localize it.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:444:
if (mApiClient != null && mApplicationStarted && mApiClient.isConnected()) {
I'm not exactly sure about the logic here. If mApplicationStarted == true, but
isConnected() == false, what should happen?

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:467:
private void showToast(String message, int duration) {
Change this to take a resource ID instead of a String message.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/re...
File remoting/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/re...
remoting/android/java/res/values/strings.xml:1: <?xml version="1.0"
encoding="utf-8"?>
Please remove this, and use our existing localization scheme instead.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File
remoting/android/java/src/org/chromium/chromoting/ActivityLifecycleListener.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/ActivityLifecycleListener.java:38:

nit: Remove blank lines

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Capabilities.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Capabilities.java:10: * they
can be enumerated here.
Does this list have to match a list defined somewhere else? If so, add a
comment.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:46: *
Constructs the Capability Manager. The same manager is used for an entire
application run.
I don't think this comment is needed.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:56: *
Returns |sInstance|. Thread-safe.
nit: Don't reference private fields inside public JavaDoc comments. Suggest:
"Returns the singleton instance."

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:60: if
(sInstance == null) {
nit: the lines inside synchronized {...} should be indented +4 spaces.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:68: *
Returns a space-separated list (required by host) of the capabilities supported
using
nit: Same here. Suggest: "... supported by this client."

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:80: *
events., thus supporting |capability|. This allows extensions like the
CastExtensionHandler
Not sure what "thus supporting |capability|" means?

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:149:
Log.w("CapabilityManager", "Failed to create CastExtensionHandler.");
Optional: Since the try {} block ends with a return, you could collapse these
identical blocks, and put the code at the end of the method instead.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:11: 
nit: remove blank lines

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:62: 
nit: only one blank line

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:68: 
nit: only one blank line

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

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:184:
prefs.getString(hostId + "_id", ""), prefs.getString(hostId + "_secret", ""),
nit: 8-space indent

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:191:
String username, String authToken, String hostJid, String hostId, String
hostPubkey,
nit: 8-space indent. Also you can begin the parameters after the '('.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:519: 
nit: only one blank line

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:521:
public static void sendExtensionMessage(String type, String data) {
Add some JavaDoc. And say which thread this should be called on.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:522: if
(!sConnected)
nit: Add braces, consistent with above.

https://codereview.chromium.org/451973002/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/451973002/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.h:52: const char* capabilities="");
Chromium style does not allow default parameters.

[aiguha, 2014-08-15 03:37:30]

PTAL.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
File remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:55:
private static final String CHROMOTOCAST_NAMESPACE =
"urn:x-cast:com.chromoting.cast.all";
On 2014/08/14 23:54:24, Lambros wrote:
> This should use "com.google.chromeremotedesktop" for official builds. You can
> add a TODO for this - don't need to fix it right now.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:141:
showToast("Failed to connect.", Toast.LENGTH_SHORT);
On 2014/08/14 23:54:24, Lambros wrote:
> Please localize any toasts. Maybe have a look through
> remoting/resources/remoting_strings.grd to see if there's already a generic
> "connection failed" message?
> 
> Also, do you need to call tearDown() here?

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:220:
public void onVolumeChanged() {} // Changes in volume do not affect us.
On 2014/08/14 23:54:24, Lambros wrote:
> nit: two spaces before //

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:263:
//
On 2014/08/14 23:54:24, Lambros wrote:
> nit: blank line above this comment block

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:311:

On 2014/08/14 23:54:24, Lambros wrote:
> nit: blank line not needed

Acknowledged.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:330:
showToast("Closing connection to Cast device.", Toast.LENGTH_SHORT);
On 2014/08/14 23:54:24, Lambros wrote:
> Do we need this toast? If so, localize it.

Acknowledged.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:444:
if (mApiClient != null && mApplicationStarted && mApiClient.isConnected()) {
On 2014/08/14 23:54:24, Lambros wrote:
> I'm not exactly sure about the logic here. If mApplicationStarted == true, but
> isConnected() == false, what should happen?

We should just have to change the mApplicationStarted to false. This tearDown()
is based off of the teardown() method here:
https://developers.google.com/cast/docs/android_sender. But flattened out at the
cost of nulling some objects that are already null or setting a false bit to
false again.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/cast/sr...
remoting/android/cast/src/org/chromium/chromoting/CastExtensionHandler.java:467:
private void showToast(String message, int duration) {
On 2014/08/14 23:54:24, Lambros wrote:
> Change this to take a resource ID instead of a String message.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/re...
File remoting/android/java/res/values/strings.xml (right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/re...
remoting/android/java/res/values/strings.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2014/08/14 23:54:24, Lambros wrote:
> Please remove this, and use our existing localization scheme instead.

Acknowledged.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File
remoting/android/java/src/org/chromium/chromoting/ActivityLifecycleListener.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/ActivityLifecycleListener.java:38:

On 2014/08/14 23:54:24, Lambros wrote:
> nit: Remove blank lines

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java
(right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:46: *
Constructs the Capability Manager. The same manager is used for an entire
application run.
On 2014/08/14 23:54:25, Lambros wrote:
> I don't think this comment is needed.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:56: *
Returns |sInstance|. Thread-safe.
On 2014/08/14 23:54:25, Lambros wrote:
> nit: Don't reference private fields inside public JavaDoc comments. Suggest:
> "Returns the singleton instance."

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:60: if
(sInstance == null) {
On 2014/08/14 23:54:25, Lambros wrote:
> nit: the lines inside synchronized {...} should be indented +4 spaces.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:68: *
Returns a space-separated list (required by host) of the capabilities supported
using
On 2014/08/14 23:54:25, Lambros wrote:
> nit: Same here. Suggest: "... supported by this client."

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:80: *
events., thus supporting |capability|. This allows extensions like the
CastExtensionHandler
On 2014/08/14 23:54:25, Lambros wrote:
> Not sure what "thus supporting |capability|" means?

Discussed offline.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/CapabilityManager.java:149:
Log.w("CapabilityManager", "Failed to create CastExtensionHandler.");
On 2014/08/14 23:54:25, Lambros wrote:
> Optional: Since the try {} block ends with a return, you could collapse these
> identical blocks, and put the code at the end of the method instead.

I wanted to do this but findbugs complains about it.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
File remoting/android/java/src/org/chromium/chromoting/Desktop.java (right):

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:11: 
On 2014/08/14 23:54:25, Lambros wrote:
> nit: remove blank lines

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:62: 
On 2014/08/14 23:54:25, Lambros wrote:
> nit: only one blank line

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/Desktop.java:68: 
On 2014/08/14 23:54:25, Lambros wrote:
> nit: only one blank line

Done.

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

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:184:
prefs.getString(hostId + "_id", ""), prefs.getString(hostId + "_secret", ""),
On 2014/08/14 23:54:25, Lambros wrote:
> nit: 8-space indent

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:191:
String username, String authToken, String hostJid, String hostId, String
hostPubkey,
On 2014/08/14 23:54:25, Lambros wrote:
> nit: 8-space indent. Also you can begin the parameters after the '('.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:519: 
On 2014/08/14 23:54:25, Lambros wrote:
> nit: only one blank line

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:521:
public static void sendExtensionMessage(String type, String data) {
On 2014/08/14 23:54:25, Lambros wrote:
> Add some JavaDoc. And say which thread this should be called on.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/android/java/sr...
remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java:522: if
(!sConnected)
On 2014/08/14 23:54:25, Lambros wrote:
> nit: Add braces, consistent with above.

Done.

https://codereview.chromium.org/451973002/diff/40001/remoting/client/jni/chro...
File remoting/client/jni/chromoting_jni_instance.h (right):

https://codereview.chromium.org/451973002/diff/40001/remoting/client/jni/chro...
remoting/client/jni/chromoting_jni_instance.h:52: const char* capabilities="");
On 2014/08/14 23:54:25, Lambros wrote:
> Chromium style does not allow default parameters.

Acknowledged.

[aiguha, 2014-08-15 03:37:31]

PTAL.

[Lambros, 2014-08-15 03:50:51]

LGTM (for a first cut!)

[aiguha, 2014-08-15 04:01:48]

The CQ bit was checked by aiguha@chromium.org

[commit-bot: I haz the power, 2014-08-15 04:08:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aiguha@chromium.org/451973002/80001

[commit-bot: I haz the power, 2014-08-15 16:19:42]

Committed patchset #5 (80001) as 289897