Closed Bug 901803 Opened 6 years ago Closed 6 years ago

Integrate chromecast support into Firefox

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 33

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webrtc][QA-])

Attachments

(5 files, 38 obsolete files)

4.06 KB, patch
Details | Diff | Splinter Review
14.06 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
17.83 KB, patch
wesj
: review+
Details | Diff | Splinter Review
8.90 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
4.80 KB, patch
nalexander
: review+
Details | Diff | Splinter Review
This is a very WIP patch to get the required libraries integrated into the Firefox for Android build system. I'm sure there are better ways to do this, and I have not handled everything.

Some initial setup:
0. General info page: https://developers.google.com/cast/devprev
1. Make sure you have the latest Android SDK updated on your developement machine. You'll need the android.support.v7.appcompat and android.support.v7.appcompat support libraries.
2. Download the Google Cast SDK from here:
   https://developers.google.com/cast/downloads/GoogleCastSdkAndroid-1.0.0.zip
   In this patch, I expect GoogleCastSdkAndroid-1.0.0.zip to be extracted here:
   <android-sdk>/extras/google/cast/<contents of zip>

Adjustment cause I don't know how to fix it:
0. Make a layout-v17 XML file hidden, so it doesn't break the build:
   extras/android/support/v7/mediarouter/res/layout-v17/mr_media_route_list_item.xml
Attached patch makefile parts v0 (obsolete) — Splinter Review
This patch adds support for the appcompat and mediarouter support libraries, and the Google Cast SDK library. I basically copied what were doing for the v4 support library. There is one big difference with the appcompat and mediarouter libraries: They include resources.

To support resources I added a $(R_JAVA_FILES) list in the Makefile, and loop over it to create R.java files for each library and Gecko. Since I needed to use folders for the library R.java files, I just moved the normal org.mozilla.gecko R.java file into a folder too.

I had hoped to be able to avoid calling AAPT for each library, but couldn't figure out how.

Once you get the libraries and hide the layout-v17 file (I wish I figured out how to fix that too), Fennec should at least build. No code will be trying to use Cast though. That's next.
Attached patch using cast v0 (obsolete) — Splinter Review
This patch tries to use the Cast code in Firefox. It doesn't really work yet, but I'm using it as a foundation.

I should note that the Google Cast API docs try to get you to use the MediaRouteButton as the UI entry point. My initial attempts in this patch do not do that. I figured that Ian would not use the simple button, jammed in our toolbar anyway. So I tried to start from scratch using the MediaRouteButton code as a blueprint.

The current patch:
1. Create the Cast and Media infrastructure in onCreate
2. Adds the selector callback in onStart. This should start the code looking for a caster.
3. Removes the callback in onStop. This should stop the system from looking for casters.
4. Adds a menu that current does nothing. Someday I hope to display the list of caster devices and send something to a receiver.


Sadly, the patch currently crashes when executing the code in onStart (step #2). The crash is:

E/GeckoAppShell(20907): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 8985 ("AsyncTask #2")
E/GeckoAppShell(20907): java.lang.IllegalStateException: Target host must not be null, or set in parameters. scheme=null, host=null, path=/apps/YouTube
E/GeckoAppShell(20907): 	at org.apache.http.impl.client.DefaultRequestDirector.determineRoute(DefaultRequestDirector.java:591)
E/GeckoAppShell(20907): 	at org.apache.http.impl.client.DefaultRequestDirector.execute(DefaultRequestDirector.java:293)
E/GeckoAppShell(20907): 	at org.apache.http.impl.client.AbstractHttpClient.execute(AbstractHttpClient.java:555)
E/GeckoAppShell(20907): 	at org.apache.http.impl.client.AbstractHttpClient.execute(AbstractHttpClient.java:487)
E/GeckoAppShell(20907): 	at android.net.http.AndroidHttpClient.execute(AndroidHttpClient.java:257)
E/GeckoAppShell(20907): 	at com.google.cast.k.a(SourceFile:166)
E/GeckoAppShell(20907): 	at com.google.cast.k.performGet(SourceFile:83)
E/GeckoAppShell(20907): 	at com.google.cast.NetworkRequest.performHttpGet(SourceFile:147)
E/GeckoAppShell(20907): 	at com.google.cast.f.execute(SourceFile:175)
E/GeckoAppShell(20907): 	at com.google.cast.NetworkTask.doInBackground(SourceFile:120)
E/GeckoAppShell(20907): 	at com.google.cast.NetworkTask.doInBackground(SourceFile:20)
E/GeckoAppShell(20907): 	at android.os.AsyncTask$2.call(AsyncTask.java:287)
E/GeckoAppShell(20907): 	at java.util.concurrent.FutureTask.run(FutureTask.java:234)
E/GeckoAppShell(20907): 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1080)
E/GeckoAppShell(20907): 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:573)
E/GeckoAppShell(20907): 	at java.lang.Thread.run(Thread.java:856)
Attached patch makefile parts v0.1 (obsolete) — Splinter Review
I forgot to move the attrs.xml change to the makefile part when I split the patch. This patch has it and will build on it's own.
Attachment #786098 - Attachment is obsolete: true
Attached patch using cast v0.1 (obsolete) — Splinter Review
Previous patch had a syntax error. I must have edited but failed to compile before posting patch. Fixed now, but the code still crashes.
Attachment #786100 - Attachment is obsolete: true
Whiteboard: [webrtc]
Attached patch Use DIAL part 1: broadcast v0.1 (obsolete) — Splinter Review
This is the begin of a completely different approach. This patch interacts with the DIAL server inside Chromecast and Leapcast. It does not use any of the Google Cast SDK. This is nice for a few reasons:
* I still could not get the Cast SDK to work correctly at runtime
* Firefox Desktop and Firefox OS can't use the Cast SDK, but could use the DIAL approach

This patch adds the UDP/Datagram broadcast required to locate DIAL servers on the local network. I would have implemented this entirely in Javascript, but Gecko does not have support for UDP client datagrams yet. See bug 745283.

So I add just enough Java code to do the broadcast and send the server info to Gecko when we find a server.
Attached patch Use DIAL part 2: JS module v0.1 (obsolete) — Splinter Review
This patch adds some Javascript classes to a DIAL.jsm module:
* DialService: once initialized, it will listen for new servers, manage the list of servers and send a Gecko notification to any internal listeners.
* DialApp: Given a server and an app name (like "YouTube), this class will allow you to start, stop and get status of the app running on the DIAL server. You can also startup a RemoteMedia connection.
* RemoteMedia: Using websockets to allow some direct control of the DialApp. The commands are app-specific, but a few have been written down.
* YoutubeApp: A simple helper that givens easy access to the DIAL name and what the data payload for starting the app looks like.
Attached patch Use DIAL part 3: Example v0.1 (obsolete) — Splinter Review
This patch is a simple example of how the DIAL code could be used. It adds a context menu menu tapping on a Youtube like or preview. Choosing "Cast to YouTube" will cause the video to start playing on the Leapcast or Chromecast server.
Attached patch Use DIAL part 1: broadcast v0.2 (obsolete) — Splinter Review
CheapCast did not use /r/n to separate headers. It uses /n, so this patch tries to be more lenient and accept either.
Attachment #789171 - Attachment is obsolete: true
Comment on attachment 786102 [details] [diff] [review]
makefile parts v0.1

Obsoleting the Cast SDK patches since they don't work and are tied to Android SDKs.
Attachment #786102 - Attachment is obsolete: true
Attachment #786104 - Attachment is obsolete: true
Note: If you don't have a Chromecast, but want to play with the technology (using Google Music and YouTube, not just Fennec) try using:
* Leapcast: Works on desktops https://github.com/dz0ny/leapcast/
* Leapcast packaged for Mac: http://wolfpaulus.com/jounal/mac/chromecasting
* Cheapcast: Android app for Tablets and Google TV https://play.google.com/store/apps/details?id=at.maui.cheapcast
Whiteboard: [webrtc] → [webrtc][QA-]
Attached patch Use DIAL part 1: broadcast v0.3 (obsolete) — Splinter Review
Minor fixup: Removed unneeded trim()
Assignee: nobody → mark.finkle
Attachment #789770 - Attachment is obsolete: true
Attached patch Use DIAL part 2: JS module v0.2 (obsolete) — Splinter Review
Minor changes:
* friendly -> friendlyName for server
* added resourceURL and instanceURL to DialApp
* removed some logging
Attachment #789173 - Attachment is obsolete: true
Attached patch Use DIAL part 3: Example v0.2 (obsolete) — Splinter Review
Updated:
* added a way select the Cast server if you have more than one
* updated code to use friendlyName
Attachment #789175 - Attachment is obsolete: true
Flags: sec-review?
Attached patch Use DIAL part 2: JS module v0.3 (obsolete) — Splinter Review
Adds a listener callback for the RemoteMedia object
Attachment #790196 - Attachment is obsolete: true
Attached patch Use DIAL part 3: Example v0.3 (obsolete) — Splinter Review
Refresh
Attachment #790199 - Attachment is obsolete: true
This patch adds simple "tab mirroring" features. Basic flow:
1. Start by choosing "Cast this Tab" from the menu
2. Pick a cast server that supports mirroring (more later)
3. As you browse, an image of the active tab's content is pushed to the cast server
4. Choose "Cast this Tab" to turn it off

Details:
Cast APIs support sending a URL to a Cast App (a Chrome App running on the cast server) which the app can use to do what it needs to do. I tried sending a data: URI of the screenshot, but it was too big and it didn't work. So I switched gears and created a super simple, single purpose HTTP server running on a non-standard port. The HTTP server only holds the current screenshot and will send it to any request, ignoring anything else sent from the client.

Now, I send this unique URL to the Cast App when the tab content changes and it will request the screenshot. It works. I use tabSelect and MozAfterPaint (throttled) as triggers for sending the screenshot.

TODO:
1. I have the server IP hardcoded right now cause I can't find a way in Gecko to get the localhost IP. nsIDNSService should work, but it's not.
2. Fix menu strings. Use real reources and add a "Stop Casting" string
3. Get some feedback on the super simple HTTP server.
Attached file Mirroring cast app v0.1 (obsolete) —
The Tab Mirroring patch depends on a Cast App called "Finkle". This is the current snapshot of that app. It's pretty basic. If you have Cheapcast installed, you can enable "Custom Services" and register the "Finkle" app. Register using a terminal window and curl:
curl --data "{'appName':'Finkle', 'appUrl': 'http://people.mozilla.com/~mfinkle/castapp.html?$query', 'protocols': [ 'ramp' ]}" http://192.168.10.128:8008/registerApp

where "192.168.10.128" is the IP of your Cheapcast device

I am hosting it (yes it must be hosted) here:
http://people.mozilla.com/~mfinkle/castapp.html
Attached patch Use DIAL part 1: broadcast v0.4 (obsolete) — Splinter Review
Rebased to mozilla-inbound
Attachment #790195 - Attachment is obsolete: true
Attached patch Use DIAL part 2: JS module v0.4 (obsolete) — Splinter Review
Rebased to mozilla-inbound
Attachment #792349 - Attachment is obsolete: true
Attached patch Use DIAL part 3: Example v0.4 (obsolete) — Splinter Review
* Rebased to mozilla-inbound
* Stopped using hardcoded strings. Added strings to browser.properties
Attachment #792350 - Attachment is obsolete: true
* Rebased to mozilla-inbound
* Moved main menu code to Java instead of JS
* Added Cast icon for menu
* Made the menu a toggle
* Added a CastingState to BrowserApp so we know if a DIAL server is even available and if we are actively mirroring
* Used CastingState to hid the menu if no server is available
* Using proper string resources
* Freeing the canvas memory to reduce memory usage. Needed for animated web content, which was using too much memory and GC wasn't freeing fast enough.
* Changed the MozAfterPaint throttling to render at 500ms chunks. This allows animated web content to be mirrored, but doesn't kill the device.
Attachment #792356 - Attachment is obsolete: true
Updated build is here:
http://people.mozilla.com/~mfinkle/fennec/fennec-26.0a1.en-US.android-arm.casting.apk

Remember, "Cast to Device" menu won't work unless you also have setup the "castapp.html" (Finkle) app too. You should be able to "Cast to YouTube" when long tapping YouTube videos as long as you have a Chromecast, Leapcast or Cheapcast.
I believe this is appropriate to leave here:
"Google blocks Chromecast app that let you stream your own videos" http://www.theverge.com/2013/8/25/4657202/google-blocks-chromecast-app-that-let-you-stream-own-videos

Google may be working to actively block third party casters.
Forgot to qref
Attachment #795055 - Attachment is obsolete: true
(In reply to Mark S. from comment #24)
> I believe this is appropriate to leave here:
> "Google blocks Chromecast app that let you stream your own videos"
> http://www.theverge.com/2013/8/25/4657202/google-blocks-chromecast-app-that-
> let-you-stream-own-videos
> 
> Google may be working to actively block third party casters.

Google has issued a statement, which you can find in that link now. I don't believe Google will block 3rd party casters that use their own receiver app, which the "Finkle" (castapp.html) is an example. We'll know more as the SDK comes out of beta.
it probably makes sense to wait on review until the SDK shows up unless we have some other internal support
Flags: sec-review?
Blocks: 921924
In order to support other platforms, I made this patch more general. It's now basic SSDP, with support for trying a given "target". DIAL (for chromecast) can be one of those targets.

The code waits for a request to search from JS, then broadcasts a ping and waits a bit for a return. The broadcast and the listener are are separate threads. If the listener finds a valid return, it forwards the information to JS.

The patch works, but I could use some feedback on the design, threading and UDP code.
Attachment #795046 - Attachment is obsolete: true
Attachment #811801 - Flags: feedback?(rnewman)
This patch now breaks up the SimpleServiceDiscovery (SSPD) part and the ChromecastApp part into separate JSMs.

The SSDP part could be a little friendlier to adding support for new services. I plan to do that in later patches, in other bugs.

The SimpleServiceDiscovery JSM can "search" for specific services using the "target" param. That is part of the SSDP spec. ChromecastApp is specific to the way Chromecast protocols work. The RemoteMedia class is a wrapper around the RAMP spec.

I am trying to use the same "interface" (think duck-typing) for the App and RemoteMedia for other services, like Roku. That is a different bug though.
Attachment #795048 - Attachment is obsolete: true
Attachment #811823 - Flags: feedback?(wjohnston)
Attachment #811823 - Flags: feedback?(margaret.leibovic)
Comment on attachment 811801 [details] [diff] [review]
Use SSDP (was DIAL) part 1: broadcast 0.5

Review of attachment 811801 [details] [diff] [review]:
-----------------------------------------------------------------

Does this mean I should order a Chromecast?

In general, I'd like to see some slightly rotated abstractions:

* Use services for lifecycle, rather than reimplementing services by using threads.
* Clarify the distinction between datagram handling, message handling, event handling, and timing. Right now these are all stirred together, rather than being usefully abstracted. This'll come more naturally if you build a control protocol on top of, say, ordered broadcast intents, and have a service with a datagram side and a message side. And this abstraction will allow us to tune and test how each part works, and -- if it comes to that -- allow us to build DIAL *and* other DIAL-alikes (MS SSDP for Windows Media?) on top of the same base.

::: mobile/android/base/BrowserApp.java
@@ +498,5 @@
>          JavaAddonManager.getInstance().init(getApplicationContext());
>          mSharedPreferencesHelper = new SharedPreferencesHelper(getApplicationContext());
>          mOrderedBroadcastHelper = new OrderedBroadcastHelper(getApplicationContext());
>          mBrowserHealthReporter = new BrowserHealthReporter();
> +        mSSDPSearch = new SimpleServiceDiscovery();

Why are we doing this here? Unnecessary memory footprint. See comment about service.

::: mobile/android/base/SimpleServiceDiscovery.java
@@ +19,5 @@
> +import org.apache.http.HttpResponse;
> +import org.apache.http.client.HttpClient;
> +import org.apache.http.client.methods.HttpGet;
> +import org.apache.http.Header;
> +import org.apache.http.impl.client.DefaultHttpClient;

We should probably settle on the ch.boye implementation we ship with Sync, 'cos it ain't as buggy as this one.

@@ +37,5 @@
> +import java.net.UnknownHostException;
> +
> +/* SimpleServiceDiscovery performs basic SSDP server discovery. Data is sent
> + * to Gecko, where additional discovery and management is handled.
> + * spec: http://www.dial-multiscreen.org/dial-protocol-specification

Shouldn't this be your ref?

https://tools.ietf.org/html/draft-cai-ssdp-v1-03

@@ +74,5 @@
> +        dispatcher.unregisterEventListener("SSDP:Search", this);
> +    }
> +
> +    @Override
> +    public void handleMessage(final String event, final JSONObject message) {

This is called on the thread that's dispatching events. `search` shouldn't do *any* work on this thread. See below.

@@ +77,5 @@
> +    @Override
> +    public void handleMessage(final String event, final JSONObject message) {
> +        if (event.equals("SSDP:Search")) {
> +            try {
> +                String target = message.isNull("target") ? null : message.getString("target");

message.optString("target");

or

  if (!message.has("target")) {
      return;
  }

@@ +89,5 @@
> +    }
> +
> +    protected void search(final String target) {
> +        final ConnectivityManager connMgr = (ConnectivityManager) GeckoAppShell.getContext().getSystemService(Context.CONNECTIVITY_SERVICE);
> +        final NetworkInfo netInfo = connMgr.getActiveNetworkInfo();

A lot of this is logic that we already implement in the background services module (or intend to, such as watching of connectivity status to stop and start network activity, rather than this kind of polling). And below you're already doing work to spawn threads and try to keep them aligned, driven by message passing, which is exactly what a service is.

So this seems like a good place to use an actual Android service, rather than a thread inside Fennec, no? (One could easily imagine Android providing this service in a future release, which is a decent rule of thumb.)

So: lift this out? Your Gecko messages -- "start looking", "stop looking" -- would basically get caught by a background service (doing its own thread management and queuing of actions) that would do all of the multicast nonsense, sending intents back (or otherwise queuing events) for Gecko to consume.

@@ +136,5 @@
> +
> +        // Create a thread to listen for SSDP server responses. As we find
> +        // servers, we send a message to Gecko to handle the next phases of
> +        // discovery.
> +        new Thread(new Runnable() {

I think there's a tighter lifecycle relationship between these two threads than you have implemented. For example, if one dies (OOM during creation?), should you start the other? Which should you start first? If Gecko decides to not bother (user hits back), they both need to be cleaned up. If one is interrupted, what happens? Etc.

@@ +157,5 @@
> +                        for (int i = 0; i < tokens.length; i++) {
> +                            String token = tokens[i];
> +                            String header = token.toUpperCase();
> +                            if (header.startsWith(HEADER_LOCATION)) {
> +                                location = token.substring(10).trim();

All of this should be split out, made super super defensive, and aggressively tested -- it's raw network input. Eventually.
Attachment #811801 - Flags: feedback?(rnewman) → feedback+
This patch is just rebased on part 1 and part 2. It uses the other code to implement casting a Youtube video to a Chromecast (DIAL) server device.
Attachment #795049 - Attachment is obsolete: true
Attached patch Tab mirroring (part 4) v0.4 (obsolete) — Splinter Review
This patch is rebased on the previous patches. One change to this patch is that I moved the MirrorServer out into it's own JSM. This makes it easier to use it with other casting servers.
(In reply to Richard Newman [:rnewman] from comment #30)

> Does this mean I should order a Chromecast?

You haven't already?

> * Use services for lifecycle, rather than reimplementing services by using
> threads.

Using a service seems like a good idea

> * Clarify the distinction between datagram handling, message handling, event
> handling, and timing. Right now these are all stirred together, rather than
> being usefully abstracted. This'll come more naturally if you build a
> control protocol on top of, say, ordered broadcast intents, and have a
> service with a datagram side and a message side. And this abstraction will
> allow us to tune and test how each part works, and -- if it comes to that --
> allow us to build DIAL *and* other DIAL-alikes (MS SSDP for Windows Media?)
> on top of the same base.

Maybe? I guess I never mentioned it, but I want to kill this code eventually by moving the discovery into Gecko. Gecko lacks the UDP multicast broadcast support or I already would have moved it. I definitely want to keep the core logic for different systems (Chromecast, Roku) in JS if possible.

I imagine some cleanup will happen as we move the little bit of code into a service though, but I'd like to avoid any non-trivial protocols.

> > + * spec: http://www.dial-multiscreen.org/dial-protocol-specification
> 
> Shouldn't this be your ref?
> 
> https://tools.ietf.org/html/draft-cai-ssdp-v1-03

It can be listed for sure, but honestly, I found both Chromecast and Roku deviate slightly. More the merrier!

> > +    public void handleMessage(final String event, final JSONObject message) {
> 
> This is called on the thread that's dispatching events. `search` shouldn't
> do *any* work on this thread. See below.

It does *very* little work on the dispatch thread, but moving this to a service makes this moot.

> > +    @Override
> > +    public void handleMessage(final String event, final JSONObject message) {
> > +        if (event.equals("SSDP:Search")) {
> > +            try {
> > +                String target = message.isNull("target") ? null : message.getString("target");
> 
> message.optString("target");
> 
> or
> 
>   if (!message.has("target")) {
>       return;
>   }

Yes, that's better

> @@ +89,5 @@
> > +    }
> > +
> > +    protected void search(final String target) {
> > +        final ConnectivityManager connMgr = (ConnectivityManager) GeckoAppShell.getContext().getSystemService(Context.CONNECTIVITY_SERVICE);
> > +        final NetworkInfo netInfo = connMgr.getActiveNetworkInfo();
> 
> A lot of this is logic that we already implement in the background services
> module (or intend to, such as watching of connectivity status to stop and
> start network activity, rather than this kind of polling). And below you're
> already doing work to spawn threads and try to keep them aligned, driven by
> message passing, which is exactly what a service is.

Moving to a thread is a good idea, but I'm not sure how much of the existing services code we can just use. The Wifi check could probably be used since we only want to look for devices on the same local network and if you're not on a local network, we should bail.

I'm not convinced we wouldn't keep using the polling though, since Chromecast/Roku devices can come on and offline too - not just your phone.

> So this seems like a good place to use an actual Android service, rather
> than a thread inside Fennec, no? (One could easily imagine Android providing
> this service in a future release, which is a decent rule of thumb.)

Yeah, as long as we keep this in Java code, a Service sounds fine. I *do* want to move this into Gecko itself though. Supporting this feature on desktop and firefoxos means support in Gecko is needed.

> So: lift this out? Your Gecko messages -- "start looking", "stop looking" --
> would basically get caught by a background service (doing its own thread
> management and queuing of actions) that would do all of the multicast
> nonsense, sending intents back (or otherwise queuing events) for Gecko to
> consume.

Sounds like a plan

> > +        // Create a thread to listen for SSDP server responses. As we find
> > +        // servers, we send a message to Gecko to handle the next phases of
> > +        // discovery.
> > +        new Thread(new Runnable() {
> 
> I think there's a tighter lifecycle relationship between these two threads
> than you have implemented. For example, if one dies (OOM during creation?),
> should you start the other? Which should you start first? If Gecko decides
> to not bother (user hits back), they both need to be cleaned up. If one is
> interrupted, what happens? Etc.

Given the threads do so little, I probably don't care what happens to them if Gecko is killed/OOM. Back/Forward is not a concern either, since the only purpose here is a discovery ping, which is global to any particular page. As long as neither thread is left to run indefinitely, we should be OK.
Comment on attachment 811823 [details] [diff] [review]
Use SSDP (was DIAL) part 2: JS modules v0.5

Review of attachment 811823 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/ChromecastApp.jsm
@@ +17,5 @@
> +
> +/* ChromecastApp is a wrapper for interacting with a DIAL server.
> + * The basic interactions all use a REST API.
> + */
> +function ChromecastApp(aServer, aApp) {

I was hoping this code would be a bit more general with a specific "Chromecast" implementation on top. I'm not sure if that's possible (or useful) or not.

@@ +32,5 @@
> +    xhr.open("GET", this.resourceURL, true);
> +    xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> +    xhr.overrideMimeType("text/xml");
> +
> +    xhr.addEventListener("load", (function() {

You could use a fat arrow here ( () => ) and avoid the bind.

@@ +100,5 @@
> +          aCallback(false);
> +      }
> +    }).bind(this), false);
> +
> +    xhr.addEventListener("error", (function() {

This pattern is repeated enough (xhr.addEventListener("load") { ... calback() stuff... } xhr.addEventListener("error") {  ... callback() stuff... }. Make sense to break it out.

@@ +137,5 @@
> +        let data = {
> +          channel: 0,
> +          senderId: {
> +            appName: "ChromeCast",
> +            senderId: "7v3zqrpliq3i"

i.e. see my comment about being general above. Can we make this class generic and have implementations inherit from it?

@@ +233,5 @@
> +  },
> +
> +  onServerClose: function(aContext, aStatusCode, aReason) {
> +    this._active = false;
> +    this._timer.cancel();

Do you need to close the web socket here?

@@ +247,5 @@
> +}
> +
> +YoutubeMeta.prototype = {
> +  get name() {
> +    return "YouTube";

Why is this a getter?

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +16,5 @@
> +}
> +
> +function sendMessageToJava(message) {
> +  return Cc["@mozilla.org/android/bridge;1"]
> +    .getService(Ci.nsIAndroidBridge)

Services.androidBridge

@@ +34,5 @@
> +
> +  search: function search() {
> +    Services.obs.addObserver(this, "SSDP:FoundServer", false);
> +    sendMessageToJava({ type: "SSDP:Search", target: "roku:ecp" });
> +    sendMessageToJava({ type: "SSDP:Search", target: "urn:dial-multiscreen-org:service:dial:1" });

This seems... less than great. Can this take a target to search for, or pass in an array? At least maybe these could be a field in SimpleServiceDicscovery that we loop over.

@@ +78,5 @@
> +        server.manufacturer = doc.querySelector("manufacturer").textContent;
> +        server.modelName = doc.querySelector("modelName").textContent;
> +
> +        // Only add and notify if we don't already know about this server
> +        if (this._servers.hasOwnProperty(server.location) == false) {

This is weird. If you're worried about falsiness, maybe you need triple equals? Comments for some of this would be nice :)
Attachment #811823 - Flags: feedback?(wjohnston) → feedback+
Comment on attachment 811823 [details] [diff] [review]
Use SSDP (was DIAL) part 2: JS modules v0.5

Review of attachment 811823 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't look through the details of the different protocols, but this seems sensible. I also agree with everything wesj had to say.

::: mobile/android/modules/ChromecastApp.jsm
@@ +20,5 @@
> + */
> +function ChromecastApp(aServer, aApp) {
> +  this.server = aServer;
> +  this.app = aApp;
> +  this.resourceURL = this.server.appsURL + "/" + this.app;

It doesn't look like you use this.server or this.app for anything else, so I don't think you need to save them (unless you're going to use them for something in the future).

@@ +25,5 @@
> +  this.instanceURL = null;
> +}
> +
> +ChromecastApp.prototype = {
> +  status: function status(aCallback) {

I've grown to dislike the aFoo parameter convention. But I don't care enough to make you change things if you don't want to.

@@ +37,5 @@
> +      if (xhr.status == 200) {
> +        let doc = xhr.responseXML;
> +        let state = doc.querySelector("state").textContent;
> +        let serviceURL = doc.querySelector("connectionSvcURL").textContent;
> +        if (aCallback)

Is it ever useful call status without aCallback? It doesn't look like the method does anything interesting in that case. If not, maybe you could just bail early and report an error if aCallback is null/undefined, instead of having all these if statements. Same idea applies to the other methods that take callbacks.

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +39,5 @@
> +  },
> +
> +  get servers() {
> +    let array = [];
> +    for each(let server in this._servers)

for each ... in is deprecated, so you shouldn't use it. Instead you can iterate through Object.keys(this._servers).
Attachment #811823 - Flags: feedback?(margaret.leibovic) → feedback+
(In reply to Wesley Johnston (:wesj) from comment #34)

> ::: mobile/android/modules/ChromecastApp.jsm

> > +/* ChromecastApp is a wrapper for interacting with a DIAL server.
> > + * The basic interactions all use a REST API.
> > + */
> > +function ChromecastApp(aServer, aApp) {
> 
> I was hoping this code would be a bit more general with a specific
> "Chromecast" implementation on top. I'm not sure if that's possible (or
> useful) or not.

It's not really possible. Each system is fairly different. If other devices start supporting the Chromecast dialect of DIAL and RemoteMedia, it might be possible. For an idea of how I am working around that, see bug 921948 for a Roku variant of an object with the same "interface" - JS + duck typing FTW.


> @@ +32,5 @@
> > +    xhr.open("GET", this.resourceURL, true);
> > +    xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
> > +    xhr.overrideMimeType("text/xml");
> > +
> > +    xhr.addEventListener("load", (function() {
> 
> You could use a fat arrow here ( () => ) and avoid the bind.

I could, but I am old-school

> @@ +100,5 @@
> > +          aCallback(false);
> > +      }
> > +    }).bind(this), false);
> > +
> > +    xhr.addEventListener("error", (function() {
> 
> This pattern is repeated enough (xhr.addEventListener("load") { ...
> calback() stuff... } xhr.addEventListener("error") {  ... callback()
> stuff... }. Make sense to break it out.

I might be able to make something that could be reused, but I won't bend over backwards to do it. I don't like forcing reuse if the solution sucks or is hard to understand/maintain.

> > +        let data = {
> > +          channel: 0,
> > +          senderId: {
> > +            appName: "ChromeCast",
> > +            senderId: "7v3zqrpliq3i"
> 
> i.e. see my comment about being general above. Can we make this class
> generic and have implementations inherit from it?

Nope. There is nothing to inherit. But we need to see if Mozilla will get it's own data for a call like this.

> > +  onServerClose: function(aContext, aStatusCode, aReason) {
> > +    this._active = false;
> > +    this._timer.cancel();
> 
> Do you need to close the web socket here?

I don't know. The "docs" are vague, but I can dig a bit more.

> > +YoutubeMeta.prototype = {
> > +  get name() {
> > +    return "YouTube";
> 
> Why is this a getter?

No reason. I honestly don't know if I even want to make these "meta" objects.

> ::: mobile/android/modules/SimpleServiceDiscovery.jsm

> > +function sendMessageToJava(message) {
> > +  return Cc["@mozilla.org/android/bridge;1"]
> > +    .getService(Ci.nsIAndroidBridge)
> 
> Services.androidBridge

\o/

> > +  search: function search() {
> > +    Services.obs.addObserver(this, "SSDP:FoundServer", false);
> > +    sendMessageToJava({ type: "SSDP:Search", target: "roku:ecp" });
> > +    sendMessageToJava({ type: "SSDP:Search", target: "urn:dial-multiscreen-org:service:dial:1" });
> 
> This seems... less than great. Can this take a target to search for, or pass
> in an array? At least maybe these could be a field in
> SimpleServiceDicscovery that we loop over.

Yeah, this is not my favorite code. An array sounds nice. I was actually thinking of having the "App" classes register their SSDP target *and* a method for returning an App class. That way a developer doesn't need to worry about a ChromecastApp or a RokuApp and what type of target the server has.

I'll tweak this for next review.

> > +        server.manufacturer = doc.querySelector("manufacturer").textContent;
> > +        server.modelName = doc.querySelector("modelName").textContent;
> > +
> > +        // Only add and notify if we don't already know about this server
> > +        if (this._servers.hasOwnProperty(server.location) == false) {
> 
> This is weird. If you're worried about falsiness, maybe you need triple
> equals? Comments for some of this would be nice :)

I could yank the "== false" (too much Java coding)
(In reply to :Margaret Leibovic from comment #35)
> Comment on attachment 811823 [details] [diff] [review]
> Use SSDP (was DIAL) part 2: JS modules v0.5
> 
> Review of attachment 811823 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I didn't look through the details of the different protocols, but this seems
> sensible. I also agree with everything wesj had to say.
> 
> ::: mobile/android/modules/ChromecastApp.jsm
> @@ +20,5 @@
> > + */
> > +function ChromecastApp(aServer, aApp) {
> > +  this.server = aServer;
> > +  this.app = aApp;
> > +  this.resourceURL = this.server.appsURL + "/" + this.app;
> 
> It doesn't look like you use this.server or this.app for anything else, so I
> don't think you need to save them (unless you're going to use them for
> something in the future).

Exactly. Plus I want to reuse the pattern of passing these params to any and all "App" classes, like the RokuApp in bug 921948.

> > +ChromecastApp.prototype = {
> > +  status: function status(aCallback) {
> 
> I've grown to dislike the aFoo parameter convention. But I don't care enough
> to make you change things if you don't want to.

Change to what? aFoo -> foo ? Phooey.

> > +      if (xhr.status == 200) {
> > +        let doc = xhr.responseXML;
> > +        let state = doc.querySelector("state").textContent;
> > +        let serviceURL = doc.querySelector("connectionSvcURL").textContent;
> > +        if (aCallback)
> 
> Is it ever useful call status without aCallback? It doesn't look like the
> method does anything interesting in that case. If not, maybe you could just
> bail early and report an error if aCallback is null/undefined, instead of
> having all these if statements. Same idea applies to the other methods that
> take callbacks.

In this case, it also sets the connectionSvcURL URL which might be useful, but I agree. It should probably treat it as an error.

> ::: mobile/android/modules/SimpleServiceDiscovery.jsm

> > +  get servers() {
> > +    let array = [];
> > +    for each(let server in this._servers)
> 
> for each ... in is deprecated, so you shouldn't use it. Instead you can
> iterate through Object.keys(this._servers).

LOL. Sure.
Attachment #796064 - Attachment is obsolete: true
Comment on attachment 811801 [details] [diff] [review]
Use SSDP (was DIAL) part 1: broadcast 0.5

Bug 938571 focuses on the core system now
Attachment #811801 - Attachment is obsolete: true
Comment on attachment 811823 [details] [diff] [review]
Use SSDP (was DIAL) part 2: JS modules v0.5

Bug 938571 focuses on the core system now
Attachment #811823 - Attachment is obsolete: true
Depends on: 938571
For additional clarity, the above link (in comment #40 ) is to an article announcing that Google has opened the Chromecast SDK to developers.
(In reply to Ahmed Nefzaoui [:Nefzaoui] from comment #40)
> Wouldn't that be a little helpful ?
> http://www.engadget.com/2014/02/03/google-opens-chromecast-sdk/

Not as helpful as you might think. We do not intend to build support for Chromecast using the Android API. We intend to use an open approach, so we can use it on Android, Desktop and FirefoxOS.
Is there a reason, why you are doing this in Firefox and not in a separate project, so others may use it, too?
Attached patch WIP Patch (obsolete) — Splinter Review
This imports GooglePlay Services along with the v7 libraries (and resources) and builds on the patch in bug 992964 to add ChromeCast support for playing videos on the remote display. Very WIP. Saving my place. It uses the built in Media Receiver on the chromecast so we don't need an app there :)

I noticed a sample in the play sdk that uses intents for most of its casting. That may provide a better way forward here (I'm curious if we can use the Strings and avoid importing the play sdk entirely...)
Out of curiosity, what are the use cases you're working on in this bug? Is there a UX spec?

I see talk of tab casting and something that sounds specific to YouTube? Is it your intention that Firefox users would see cast icons appear inside web pages like desktop Chrome users do with the Chromecast extension, or are you looking for a more generic approach via context menus and/or intents?

Do you think any of this code can be re-used in Firefox OS?
(In reply to Ben Francis [:benfrancis] from comment #45)

> Do you think any of this code can be re-used in Firefox OS?

No because it uses the Android SDK for that. Sadly when Google implement things these day they do like Microsoft: they implement for their browser / platform / apps. Not for the web. :-(
(In reply to Ben Francis [:benfrancis] from comment #45)
> I see talk of tab casting and something that sounds specific to YouTube? Is
> it your intention that Firefox users would see cast icons appear inside web
> pages like desktop Chrome users do with the Chromecast extension, or are you
> looking for a more generic approach via context menus and/or intents?

The way this is currently implemented there is a context menu item on all HTML5 video elements that allows casting. We're also looking at a cast icon in the urlbar for situations where the context menu in inaccessible or has has been disabled on a video (I need to review that today).

Like Hubert said, Chromecast isn't open source. Getting it to work on desktop would require some reverse engineering and may or may not be impossible (I've heard that the Desktop extension for Chrome is all JS though, so that code may be useable).

More generally, we've built a framework that allows adding backends by us or extensions. We're hoping to support as many we can that have standard, stable specs. Some of those are shareable amongst Desktop Firefox, Firefox on Android and Firefox OS, but some will be platform dependent (AppleTV for example).
I read the DIAL specification a while back [1]. How much of this implementation is implementing the DIAL protocol, and how much of it is Chromecast-specific?

It seems like the UPnP part could be challenging to implement in Firefox OS?

As I understand it, all DIAL "1st screen" application names (the app displayed on the TV) have to be registered in the DIAL registry [2] which appears to be maintained by someone at Netflix. All "Chromecast receiver apps" (the Chromecast web app implementation attached to these app names) have to be registered with Google via the Google Cast SDK Developer Console [3] (which costs $5 and provides the developer with an app ID) in order to map DIAL app names onto Chromecast receiver apps.

This probably isn't a problem for this client-side implementation because it seems you're using Google's default media receiver app on the Chromecast which is already registered, but I assume people building casting functionality into their web apps for use in Firefox would need to register their apps with Google? Would we even support those use cases?

If DIAL (and by extension Chromecast) require a central registry of application names maintained by Netflix & Google as gatekeepers, should we be implementing that protocol or should we be proposing an alternative solution which is more open and web-like?

1. http://www.dial-multiscreen.org/
2. http://www.dial-multiscreen.org/dial-registry
3. https://cast.google.com/publish
4. https://developers.google.com/cast/docs/reference/chrome/
(In reply to Ben Francis [:benfrancis] from comment #48)
> I read the DIAL specification a while back [1]. How much of this
> implementation is implementing the DIAL protocol, and how much of it is
> Chromecast-specific?

None of it is Chromecast specific, and we already landed a JS implementation of SSDP. You use SSDP to find the Chromecast and DIAL to find the "apps URL", which looks like this:

"http://10.251.34.240:8008/apps/"

You can use DIAL to launch YouTube on a Chromecast like this:

// Start a given app with any extra query data. Each app uses it's own data scheme.
let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].createInstance(Ci.nsIXMLHttpRequest);
xhr.open("POST", "http://10.251.34.240:8008/apps/YouTube", true);
xhr.channel.loadFlags |= Ci.nsIRequest.INHIBIT_CACHING;
xhr.setRequestHeader("Content-Type", "application/x-www-form-urlencoded;charset=UTF-8");
xhr.setRequestHeader("Origin", "chrome-extension://boadgeojelhgndaghljhdicfkmllpafd");

xhr.addEventListener("load", (function() {
  if (xhr.status == 201) {
      console.log("worked")
  } else {
      console.log("failed " + xhr.status)
  }
}).bind(this), false);

xhr.addEventListener("error", (function() {
  console.log("error")
}).bind(this), false);

let videoKey = "6gotuI94NOo";
xhr.send("pairingCode=428651d1-26ca-410a-8050-aaaab53a9582&v=4ThvBNZdGcQ&t=10");
(In reply to Mark Finkle (:mfinkle) from comment #49)
> (In reply to Ben Francis [:benfrancis] from comment #48)

Notes:

> xhr.setRequestHeader("Origin",
> "chrome-extension://boadgeojelhgndaghljhdicfkmllpafd");

This part is not documented and has changed a few time.

> let videoKey = "6gotuI94NOo";
> xhr.send("pairingCode=428651d1-26ca-410a-8050-
> aaaab53a9582&v=4ThvBNZdGcQ&t=10");

This part is less-than-documented
Oh, I didn't list any of the "can't reverse engineer" parts:
1. Launching your own app, registered with Google, doesn't work like the YouTube example. We don't really know why.
2. Chromecast apps require the device to become the remote. A Chromecast does not have a physical remote control. The device and the app need to connect. Chromecasts has a built-in protocol that allows this called RemoteMedia. The connection is based on WebSockets, but the connection doesn't seem to happen without some secret sauce in the SDK.
 
> > let videoKey = "6gotuI94NOo";
> > xhr.send("pairingCode=428651d1-26ca-410a-8050-
> > aaaab53a9582&v=4ThvBNZdGcQ&t=10");
> 
> This part is less-than-documented

Also I think you meant 

let videoKey = "6gotuI94NOo";
xhr.send("pairingCode=428651d1-26ca-410a-8050-aaaab53a9582&v=" + videoKey + "&t=10");


Didn't you? ;-) For the sake of completness.
Maybe related to this issue: Bug 914579 - Add Network Service Discovery API support
Comment on attachment 8414286 [details] [diff] [review]
WIP Patch

Review of attachment 8414286 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by on the build system parts of this.  Overall, I'm pleasantly surprised at how easy this was.  (I did it locally before realizing you'd worked out some of these things slightly differently already.)  The build parts of this are 90% there!

::: build/autoconf/android.m4
@@ +323,5 @@
>      fi
>  
>      ANDROID_SDK="${android_sdk}"
>      ANDROID_SDK_ROOT="${android_sdk_root}"
> +    GOOGLE_PLAY_SERVICES="${ANDROID_SDK_ROOT}/extras/google/google_play_services/libproject/google-play-services_lib/libs/google-play-services.jar";

I'm not sure we want to require GPS to build.  (Requiring v4 and v7 copmat libraries seems fine.)

::: config/android-common.mk
@@ +19,5 @@
>    $(NULL)
>  
>  # For Android, this defaults to $(ANDROID_SDK)/android.jar
>  ifndef JAVA_BOOTCLASSPATH
> +  JAVA_BOOTCLASSPATH = $(ANDROID_SDK)/android.jar:$(ANDROID_COMPAT_LIB):$(ANDROID_APPCOMPAT_LIB):$(ANDROID_MEDIAROUTER_LIB):$(GOOGLE_PLAY_SERVICES)

Don't do this; this is used in a bunch of places.  You should be able to tweak this locally; and indeed, base/Makefile.in does so.

::: mobile/android/base/Makefile.in
@@ +64,5 @@
> +ANDROID_MEDIAROUTER_RES = $(ANDROID_SDK)/../../extras/android/support/v7/mediarouter/res
> +GOOGLE_PLAY_SERVICES_RES = $(ANDROID_SDK)/../../extras/google/google_play_services/libproject/google-play-services_lib/res
> +
> +R_JAVA_FILES = \
> +  org.mozilla.gecko/R.java \

This is rather non-standard; we can just make this work using the existing mechanism.

@@ +94,5 @@
>  # indices.
>  
>  classes.dex: .proguard.deps
>  	$(REPORT_BUILD)
> +	$(DX) --dex --output=classes.dex jars-proguarded $(ANDROID_COMPAT_LIB) $(ANDROID_APPCOMPAT_LIB) $(ANDROID_MEDIAROUTER_LIB) $(GOOGLE_PLAY_SERVICES)

Hmm, interesting.  This suggests we need a way to add to the set of bootclasspath classes that's not hard coded.  Setting extra_jars in moz.build should clash with this; I'm surprised this works.

@@ +100,5 @@
>  ifdef MOZ_DISABLE_PROGUARD
>    PROGUARD_PASSES=0
>  else
>    ifdef MOZ_DEBUG
> +  PROGUARD_PASSES=0

Oops :)

@@ +295,5 @@
>  		$$(addprefix -S ,$$(ANDROID_RES_DIRS)) \
> +		-S $(GOOGLE_PLAY_SERVICES_RES) \
> +		-S $(ANDROID_APPCOMPAT_RES) \
> +		-S $(ANDROID_MEDIAROUTER_RES) \
> +		--extra-packages android.support.v7.appcompat:android.support.v7.mediarouter:com.google.android.gms \

Neat!  I achieved this by calling aapt multiple times, but this is slicker.  Does this write generated/com/google/android/gms or generated/com.google.android.gms?  I really think it's the former.  If so, the dotted deps I commented on earlier should be made consistent.

::: mobile/android/base/moz.build
@@ +15,5 @@
>  resjar.sources = []
>  resjar.generated_sources += [
> +    'android/support/v7/appcompat/R.java',
> +    'android/support/v7/mediarouter/R.java',
> +    'com/google/android/gms/R.java',

I think this should be conditional.

@@ +569,5 @@
>  main.referenced_projects += [generated.name, branding.name]
>  main.extra_jars += [CONFIG['ANDROID_COMPAT_LIB']]
> +main.extra_jars += [CONFIG['ANDROID_APPCOMPAT_LIB']]
> +main.extra_jars += [CONFIG['ANDROID_MEDIAROUTER_LIB']]
> +main.extra_jars += [CONFIG['GOOGLE_PLAY_SERVICES_LIB']]

Ditto.
Attachment #8414286 - Flags: feedback+
Comment on attachment 8414286 [details] [diff] [review]
WIP Patch

Review of attachment 8414286 [details] [diff] [review]:
-----------------------------------------------------------------

A few additional comments after investigating more locally.  I'm going to file a separate ticket for these build changes; they can land (conditionally?) independent of any particular casting feature.

::: mobile/android/base/Makefile.in
@@ +64,5 @@
> +ANDROID_MEDIAROUTER_RES = $(ANDROID_SDK)/../../extras/android/support/v7/mediarouter/res
> +GOOGLE_PLAY_SERVICES_RES = $(ANDROID_SDK)/../../extras/google/google_play_services/libproject/google-play-services_lib/res
> +
> +R_JAVA_FILES = \
> +  org.mozilla.gecko/R.java \

In fact, this is unused, which is good, 'cuz I didn't like it.

@@ +148,5 @@
>  GeneratedJNIWrappers.cpp: $(ANNOTATION_PROCESSOR_JAR_FILES)
>  GeneratedJNIWrappers.cpp: $(ALL_JARS)
>  	$(JAVA) -classpath gecko-mozglue.jar:$(JAVA_BOOTCLASSPATH):$(ANNOTATION_PROCESSOR_JAR_FILES) org.mozilla.gecko.annotationProcessors.AnnotationProcessor $(ALL_JARS)
>  
> +gecko_package_dir = generated/

Ah, I see why you did this.  I think this should not have the trailing slash, and we should get rid of this entirely (and just inline generated).

@@ +168,5 @@
>  # Certain source files need to be preprocessed.  This special rule
>  # generates these files into generated/org/mozilla/gecko for
>  # consumption by the build system and IDEs.
>  
> +preprocessed := $(addsuffix .in,$(subst $(gecko_package_dir)org/mozilla/gecko/,,$(filter $(gecko_package_dir)org/mozilla/gecko/%,$(PP_JAVAFILES))))

Yeah, inlining generated is looking like a good idea.

@@ +302,2 @@
>  		-F $(3) \
>  		-J $(4) \

Fascinating aapt bug that tripped me up for a while: if the -J path is generated/org for example, only one package gets written.  If it's -J generated, everything works as expected.
> @@ +569,5 @@
> >  main.referenced_projects += [generated.name, branding.name]
> >  main.extra_jars += [CONFIG['ANDROID_COMPAT_LIB']]
> > +main.extra_jars += [CONFIG['ANDROID_APPCOMPAT_LIB']]
> > +main.extra_jars += [CONFIG['ANDROID_MEDIAROUTER_LIB']]
> > +main.extra_jars += [CONFIG['GOOGLE_PLAY_SERVICES_LIB']]
> 
> Ditto.

Note to myself: this is incorrect; the actual variable is inconsistently named GOOGLE_PLAY_SERVICES (and should be changed before landing).  I reckon we want to ANDROID_COMPAT_DIR in configure.in and the _RES and _LIB calculations locally.
> Note to myself: this is incorrect; the actual variable is inconsistently
> named GOOGLE_PLAY_SERVICES (and should be changed before landing).  I reckon
> we want to ANDROID_COMPAT_DIR in configure.in and the _RES and _LIB
> calculations locally.

Further note to self: this isn't sufficient.  Eclipse requires a dummy project to get the library resources correct.
> Further note to self: this isn't sufficient.  Eclipse requires a dummy
> project to get the library resources correct.

Further further: this will need to handle GeckoView library as well.
OS: Linux → Android
Hardware: x86_64 → All
Attached patch Patch 1/? - Native casting (obsolete) — Splinter Review
Cleaning this stuff up. This is the initial support for casting using the Native interfaces. This is just one class that listens for displays (local or remote) and if it finds one it can deal with, notifies Gecko that it exists. No display types are implemented here yet. It also handles proxying calls from Gecko to those displays via. an id.
Attachment #8424499 - Flags: review?(mark.finkle)
Attached patch Path 2/? (obsolete) — Splinter Review
This is the JS corollary to the NativeCastingService in Java. Sends off searches when they're requested. Generates objects that can proxy data to the Java services.

My chromecast disappeared in my recent move. Trying to dig it out to test this, but wanted to start feedback.
Attachment #8424523 - Flags: feedback?(mark.finkle)
Attached patch Patch 3/3 (obsolete) — Splinter Review
And this implements the Chromecast GeckoMediaPlayer. Most of this code is taken directly from the demo app. Like I said, untested right now. Asking for feedback because of that.
Attachment #792372 - Attachment is obsolete: true
Attachment #811825 - Attachment is obsolete: true
Attachment #811826 - Attachment is obsolete: true
Attachment #8414286 - Attachment is obsolete: true
Attachment #8424524 - Flags: feedback?(mark.finkle)
Blocks: 992964
Attached patch Patch 2/3 (obsolete) — Splinter Review
Updated to build. Still testing.
Attachment #8424575 - Flags: feedback?(mark.finkle)
Attached patch Patch 3/3 (obsolete) — Splinter Review
Attachment #8424523 - Attachment is obsolete: true
Attachment #8424524 - Attachment is obsolete: true
Attachment #8424523 - Flags: feedback?(mark.finkle)
Attachment #8424524 - Flags: feedback?(mark.finkle)
Attachment #8424576 - Flags: feedback?(mark.finkle)
Comment on attachment 8424499 [details] [diff] [review]
Patch 1/? - Native casting

>diff --git a/mobile/android/base/NativeCastingManager.java b/mobile/android/base/NativeCastingManager.java

I think we can do better than NativeCastingManager. Since this is a wrapper for MediaRouter and Displays, how about MediaRouteManager or simply MediaRoutes


>+import org.mozilla.gecko.util.EventCallback;
>+import org.mozilla.gecko.util.NativeEventListener;
>+import org.mozilla.gecko.util.NativeJSObject;
>+import org.json.JSONObject;
>+import org.json.JSONException;
>+
>+import java.io.IOException;
>+
>+import org.mozilla.gecko.util.EventCallback;
>+import org.mozilla.gecko.util.EventCallback;
>+import org.mozilla.gecko.util.NativeJSObject;
>+import org.json.JSONObject;
>+import org.json.JSONException;

Lots of dupes

>+import com.google.android.gms.cast.ApplicationMetadata;
>+import com.google.android.gms.cast.Cast;
>+import com.google.android.gms.cast.Cast.ApplicationConnectionResult;
>+import com.google.android.gms.cast.CastDevice;
>+import com.google.android.gms.cast.CastMediaControlIntent;
>+import com.google.android.gms.cast.MediaInfo;
>+import com.google.android.gms.cast.MediaMetadata;
>+import com.google.android.gms.cast.MediaStatus;
>+import com.google.android.gms.cast.RemoteMediaPlayer;
>+import com.google.android.gms.cast.RemoteMediaPlayer.MediaChannelResult;
>+import com.google.android.gms.common.ConnectionResult;
>+import com.google.android.gms.common.api.GoogleApiClient;
>+import com.google.android.gms.common.api.GoogleApiClient.ConnectionCallbacks;
>+import com.google.android.gms.common.api.GoogleApiClient.OnConnectionFailedListener;
>+import com.google.android.gms.common.api.ResultCallback;
>+import com.google.android.gms.common.api.Status;

Do we use all of these?

>+interface GeckoMediaPlayer {
>+    public String toJSON();
>+    public void load(String title, String url, String type, EventCallback callback);
>+    public void play(EventCallback callback);
>+    public void pause(EventCallback callback);
>+    public void stop(EventCallback callback);
>+    public String getName();

This should be OK as long as we keep it small. I wouldn't mind removing "stop" from the interface though.

>+    private NativeCastingManager(Context context) {

>+        if (context instanceof GeckoApp) {
>+            GeckoApp app = (GeckoApp) context;
>+            app.addAppStateListener(this);
>+        }

Why do we need this?

>+        EventDispatcher.getInstance().registerGeckoThreadListener(this, "Display:Load",
>+                                                                        "Display:Play",
>+                                                                        "Display:Pause",
>+                                                                        "Display:End");

I'm thinking that "Display" is too generic. What about "MediaPlayer" since you use GeckoMediaPlayer as the interface?

>+            GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("Display:Found", display.toJSON()));

For this message, I think "MediaRoute:Found" is more appropriate. It's not part of the player and is all about the route.

>+    public void onDestroy() {

Does this get called?

>+     * Here we initialize all of our profile settings, Firefox Health Report,
>+     * and other one-shot constructions.

Fix the comment

>+    /* Implementing GeckoAppShell.AppStateListener */

Why do we need this?

>+    public void onPause() {
>+        mediaRouter.removeCallback(callback);
>+
>+        if (context instanceof GeckoApp) {
>+            GeckoApp app = (GeckoApp) context;
>+            app.removeAppStateListener(this);
>+        }

Here you remove the callback and remove the AppStateListener

>+    public void onResume() {
>+        MediaRouteSelector selectorBuilder = new MediaRouteSelector.Builder()
>+            .addControlCategory(MediaControlIntent.CATEGORY_LIVE_VIDEO)
>+            .addControlCategory(MediaControlIntent.CATEGORY_REMOTE_PLAYBACK)
>+            .build();
>+        mediaRouter.addCallback(selectorBuilder, callback, MediaRouter.CALLBACK_FLAG_REQUEST_DISCOVERY);

Here you only fixup the callback. What about the AppStateListener, if we need it?

r-, but it's looking good
Attachment #8424499 - Flags: review?(mark.finkle) → review-
Are we using java code to do the discovery of Chromecasts? I think I'd rather let our js code do the discovery (either with SSDP or mDNS) even if we need the native code to actually cast to it.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #65)
> Are we using java code to do the discovery of Chromecasts? I think I'd
> rather let our js code do the discovery (either with SSDP or mDNS) even if
> we need the native code to actually cast to it.

I believe the discovery and casting parts are quite tied together. Ideally, I'd want the same thing you suggest. At the very least, I want to wrap the Java discovery in a JSM, like SSDP and mDNS, so we can treat it like it's own discovery method. Then we can replace it, if we get the chance.
Yeah. We'll still have to "discover" in Java. It would just be extra work to translate things discovered in JS to things discovered in Java. I don't really think its worth it. I'm still a bit worried actually. The "id" that I'm getting in Java right now is Android specific, so probably won't correlate exactly to the ids we have in JS (i.e. we'll probably end up with duplicate items with these patches right now).
Updated to address comments. Also, I made this conditional on GOOGLE_PLAY_SERVICES being defined at build time. That involves some reflection in BrowserApp.
Attachment #8424499 - Attachment is obsolete: true
Attachment #8429041 - Flags: review?(mark.finkle)
Attached patch Patch 1/3 (obsolete) — Splinter Review
Some small tweaks.
Attachment #8429041 - Attachment is obsolete: true
Attachment #8429041 - Flags: review?(mark.finkle)
Attachment #8429082 - Flags: review?(mark.finkle)
Attached patch Path 2/3 - Javascript component (obsolete) — Splinter Review
Javascript bits
Attachment #8424575 - Attachment is obsolete: true
Attachment #8424575 - Flags: feedback?(mark.finkle)
Attachment #8429083 - Flags: review?(mark.finkle)
Attached patch Patch 3/3 - Chromecast bits (obsolete) — Splinter Review
Attachment #8424576 - Attachment is obsolete: true
Attachment #8424576 - Flags: feedback?(mark.finkle)
Attachment #8429084 - Flags: review?(mark.finkle)
Depends on: 1016529
here's a build for UX:

http://people.mozilla.org/~wjohnston/chromecast.apk

Like I said, we don't have much control here of whats on the TV itself. We send a url and title right now. If you're interested, there are other things we could send here (track number, artist name): http://developer.android.com/reference/com/google/android/gms/cast/MediaMetadata.html Not sure what they'd all do. Not sure we'd have them for most videos.
Attached patch Patch 1/3 - MediaPlayerManager (obsolete) — Splinter Review
This is updated to the recent v7 support landing, and to address some comments from finkle.
Attachment #8429082 - Attachment is obsolete: true
Attachment #8429082 - Flags: review?(mark.finkle)
Attachment #8439579 - Flags: review?(mark.finkle)
This implements the Java side of things for Chromecast. I'm serializing basically any/all info I can get about the device to try and help with filtering. For mine I find something like:

{
	"icon":"http://192.168.1.248:8008/setup/icon.png",
	"enabled":true,
	"model":"Chromecast",
	"location":"/192.168.1.248",
	"friendlyName":"caster",
	"provider":"MediaRouter.RouteProviderInfo{ packageName=com.google.android.gms }",
	"type":"remote",
	"servicePort":8009,
	"version":"02",
	"volumeMax":20,
	"description":"Chromecast",
	"name":"caster",
	"volume":0,
	"com.google.android.gms.cast.EXTRA_CAST_DEVICE":"\"caster\" (9e291550d32d7eca524f7401330c5de2)",
	"isConnecting":false,
	"uuid":"com.google.android.gms/.cast.media.CastMediaRouteProviderService:9e291550d32d7eca524f7401330c5de2",
	"deviceId":"9e291550d32d7eca524f7401330c5de2"
}
Attachment #8429083 - Attachment is obsolete: true
Attachment #8429083 - Flags: review?(mark.finkle)
Attachment #8439581 - Flags: review?(mark.finkle)
Attached patch Patch 3/3 - Javascript (obsolete) — Splinter Review
And finally the JS bits. This removes the function target I used before and instead integrates more directly in SSDP.
Attachment #8429084 - Attachment is obsolete: true
Attachment #8429084 - Flags: review?(mark.finkle)
Attachment #8439583 - Flags: review?(mark.finkle)
Comment on attachment 8439581 [details] [diff] [review]
Patch 2/3 - Chromecast MediaPlayer

Review of attachment 8439581 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/ChromeCast.java
@@ +133,5 @@
> +
> +    public ChromeCast(Context context, RouteInfo route) {
> +        this.context = context;
> +        selectedDevice = CastDevice.getFromBundle(route.getExtras());
> +        this.route = route;

drive by: use variable prefixes
Comment on attachment 8439581 [details] [diff] [review]
Patch 2/3 - Chromecast MediaPlayer

Review of attachment 8439581 [details] [diff] [review]:
-----------------------------------------------------------------

This code as-is crashes Fennec on start up. selectedDevice is null in toJSON(). Null checking that gets me to crashing where the bundle is null. Null checking that stops the crash but leaves a blank option in the device selection menu which will crash if I select it. I'll dig in further, but just wanted to note that this shouldn't land as-is.
Attachment #8439581 - Flags: review-
this is what I've got to drop that blank entry and avoid the crashes.
Attachment #8440160 - Flags: feedback?(wjohnston)
Depends on: 1025498
Comment on attachment 8439579 [details] [diff] [review]
Patch 1/3 - MediaPlayerManager

Review of attachment 8439579 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/MediaPlayerManager.java
@@ +215,5 @@
> +        MediaRouteSelector selectorBuilder = new MediaRouteSelector.Builder()
> +            .addControlCategory(MediaControlIntent.CATEGORY_LIVE_VIDEO)
> +            .addControlCategory(MediaControlIntent.CATEGORY_REMOTE_PLAYBACK)
> +            .build();
> +        mediaRouter.addCallback(selectorBuilder, callback, MediaRouter.CALLBACK_FLAG_REQUEST_DISCOVERY);

docs recommend CALLBACK_FLAG_PERFORM_ACTIVE_SCAN whenever a device picker is shown since some devices are not discoverable with CALLBACK_FLAG_REQUEST_DISCOVERY.
Comment on attachment 8439583 [details] [diff] [review]
Patch 3/3 - Javascript

Review of attachment 8439583 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/modules/SimpleServiceDiscovery.jsm
@@ +198,5 @@
> +
> +        // Only add and notify if we don't already know about this service
> +        if (!this._services.has(service.location)) {
> +          this._services.set(service.location, service);
> +          Services.obs.notifyObservers(null, EVENT_SERVICE_FOUND, service.location);

why aren't you using registerTarget here?
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #80)
> Comment on attachment 8439583 [details] [diff] [review]
> Patch 3/3 - Javascript
> 
> Review of attachment 8439583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/SimpleServiceDiscovery.jsm
> @@ +198,5 @@
> > +
> > +        // Only add and notify if we don't already know about this service
> > +        if (!this._services.has(service.location)) {
> > +          this._services.set(service.location, service);
> > +          Services.obs.notifyObservers(null, EVENT_SERVICE_FOUND, service.location);
> 
> why aren't you using registerTarget here?

Agreed. I want to try to use registerTarget. I was planning on making a patch to change it.
I did patch it locally, but with my patch I don't seem the chromecast in the device selector. Looking further, this code is mapping the chromecasts to the location as a key, while everything else maps to the target. Not sure if that's related or not, but its getting funky.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #82)
> I did patch it locally, but with my patch I don't seem the chromecast in the
> device selector. Looking further, this code is mapping the chromecasts to
> the location as a key, while everything else maps to the target. Not sure if
> that's related or not, but its getting funky.

My tweak to wesj's patch to use registerTarget seems to work fine. Found my Chromecast and played the test video.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #80)
> Comment on attachment 8439583 [details] [diff] [review]
> Patch 3/3 - Javascript
> 
> Review of attachment 8439583 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/SimpleServiceDiscovery.jsm
> @@ +198,5 @@
> > +
> > +        // Only add and notify if we don't already know about this service
> > +        if (!this._services.has(service.location)) {
> > +          this._services.set(service.location, service);
> > +          Services.obs.notifyObservers(null, EVENT_SERVICE_FOUND, service.location);
> 
> why aren't you using registerTarget here?

I don't understand why we'd do that, especially here. We're not using targets here. We're registering a found service. (yes, the nomenclature and these interfaces are kinda awful IMO). This code is doing the same thing that _processService does (except without the XML). Mark asked me not to use targets for this patch anymore.
(In reply to Wesley Johnston (:wesj) from comment #84)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #80)
> > Comment on attachment 8439583 [details] [diff] [review]
> > Patch 3/3 - Javascript
> > 
> > Review of attachment 8439583 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/SimpleServiceDiscovery.jsm
> > @@ +198,5 @@
> > > +
> > > +        // Only add and notify if we don't already know about this service
> > > +        if (!this._services.has(service.location)) {
> > > +          this._services.set(service.location, service);
> > > +          Services.obs.notifyObservers(null, EVENT_SERVICE_FOUND, service.location);
> > 
> > why aren't you using registerTarget here?
> 
> I don't understand why we'd do that, especially here. We're not using
> targets here. We're registering a found service. (yes, the nomenclature and
> these interfaces are kinda awful IMO). This code is doing the same thing
> that _processService does (except without the XML). Mark asked me not to use
> targets for this patch anymore.

I kinda want to use registerTarget, but I didn't want to put the sendMessage call in the registerTarget call. My tweak to patch 3 makes it use registerTarget but keeps getting the list of devices in SSDP.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #77)
> Comment on attachment 8439581 [details] [diff] [review]
> Patch 2/3 - Chromecast MediaPlayer
> 
> Review of attachment 8439581 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This code as-is crashes Fennec on start up. selectedDevice is null in
> toJSON(). Null checking that gets me to crashing where the bundle is null.
> Null checking that stops the crash but leaves a blank option in the device
> selection menu which will crash if I select it. I'll dig in further, but
> just wanted to note that this shouldn't land as-is.

I'm surprised at this and I'd like to know why its happening rather than wallpaper over it. Are you using an updated play services? Is this a real chromecast or a firefly? Is your Chromecast up to date?
(In reply to Mark Finkle (:mfinkle) from comment #85)
> I kinda want to use registerTarget, but I didn't want to put the sendMessage
> call in the registerTarget call. My tweak to patch 3 makes it use
> registerTarget but keeps getting the list of devices in SSDP.

So you're passing in a secret key or something? Reviews would be more helpful to me than people writing extra patches.
Comment on attachment 8439583 [details] [diff] [review]
Patch 3/3 - Javascript

>diff --git a/mobile/android/modules/MediaPlayerApp.jsm b/mobile/android/modules/MediaPlayerApp.jsm

>+this.EXPORTED_SYMBOLS = ["MediaPlayerApp", "MediaPlayerTarget"];

You don't export the MediaPlayerTarget

>+function MediaPlayerApp(service, app) {
>+  this.service = service;
>+  this.location = service.location;
>+  this.id = service.uuid;
>+  this.app = app;
>+}

app is never used so just remove it and the this.app

>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm
>+const MEDIA_ROUTER = "MEDIA_ROUTER";

Don't bother. It only ends up in one place, and we'll likely move it at some point.

>+    // We also query Java directly here for any devices that Android might support natively (i.e. Chromecast or Miracast)
>+    sendMessageToJava({ type: "MediaPlayers:Get" }, (result) => {

My OCD does not like MediaPlayers:Get while all the rest are MediaPlayer:Xxx
Let's just go with MediaPlayer:Get

Move this code into a separate function (_searchMediaRoutes) like you had me do with fixedTargets

>+        Services.console.logStringMessage(JSON.stringify(display));

Remove before landing

>+        let service = {
>+          location: display.location,
>+          target: MEDIA_ROUTER

I like "media:router" but it's no biggie

>+        service.appsURL = display.appsURL;
>+        service.friendlyName = display.friendlyName;
>+        service.uuid = display.uuid;
>+        service.manufacturer = display.manufacturer;
>+        service.modelName = display.modelName;

Does diaply even have all of these? Let's make sure we have some data in those. But that's for a different patch review...

>     // Find the registration for the target
>-    if (this._targets.has(aService.target)) {
>+    if (aService.target === MEDIA_ROUTER) {
>+      Cu.import("resource://gre/modules/MediaPlayerApp.jsm");
>+      return new MediaPlayerApp(aService, "MediaRouter");
>+    } else if (this._targets.has(aService.target)) {

Revert this change. Let's add a target to Casting Apps, like this:


var mediaplayerTarget = {
  target: "media:router",
  factory: function(aService) {
    Cu.import("resource://gre/modules/MediaPlayerApp.jsm");
    return new MediaPlayerApp(aService, "MediaRouter");
  }
};

and then

    SimpleServiceDiscovery.registerTarget(mediaplayerTarget);

I tested this and it works for me.

r- just to see a new patch
Attachment #8439583 - Flags: review?(mark.finkle) → review-
Comment on attachment 8439579 [details] [diff] [review]
Patch 1/3 - MediaPlayerManager

>diff --git a/mobile/android/base/MediaPlayerManager.java b/mobile/android/base/MediaPlayerManager.java
>+        EventDispatcher.getInstance().registerGeckoThreadListener(this, "MediaPlayer:Load",
>+                                                                        "MediaPlayer:Start",
>+                                                                        "MediaPlayer:Stop",
>+                                                                        "MediaPlayer:Play",
>+                                                                        "MediaPlayer:Pause",
>+                                                                        "MediaPlayers:Get",
>+                                                                        "MediaPlayer:End");

MediaPlayers:Get -> MediaPlayer:Get
Attachment #8439579 - Flags: review?(mark.finkle) → review+
Comment on attachment 8439581 [details] [diff] [review]
Patch 2/3 - Chromecast MediaPlayer

>+    // This dumps everything we can find about the device into JSON. This will hopefully make it
>+    // easier to filter out duplicate devices from different sources in js.
>+    public JSONObject toJSON() {

Let's try to cut this list down a little. I'm glad you added it all here so we can debug what comes back

Let's make sure the core list items are avaliable:
friendlyName
uuid
location
modelName
manufacturer (just hardcode "Google Inc." ?)

I don't know that we need anything more than that for now and we can add more if we need it

r+ for what's here, but let's address the issue Brad was seeing too.
Attachment #8439581 - Flags: review?(mark.finkle) → review+
(In reply to Wesley Johnston (:wesj) from comment #86)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #77)
> > Comment on attachment 8439581 [details] [diff] [review]
> > Patch 2/3 - Chromecast MediaPlayer
> > 
> > Review of attachment 8439581 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This code as-is crashes Fennec on start up. selectedDevice is null in
> > toJSON(). Null checking that gets me to crashing where the bundle is null.
> > Null checking that stops the crash but leaves a blank option in the device
> > selection menu which will crash if I select it. I'll dig in further, but
> > just wanted to note that this shouldn't land as-is.
> 
> I'm surprised at this and I'd like to know why its happening rather than
> wallpaper over it. Are you using an updated play services? Is this a real
> chromecast or a firefly? Is your Chromecast up to date?

I should note that the log output showed that the friendlyName was being sent to JS, so the if (selectedDevice) { block was being entered in my builds.
(In reply to Wesley Johnston (:wesj) from comment #86)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #77)
> > Comment on attachment 8439581 [details] [diff] [review]
> > Patch 2/3 - Chromecast MediaPlayer
> > 
> > Review of attachment 8439581 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This code as-is crashes Fennec on start up. selectedDevice is null in
> > toJSON(). Null checking that gets me to crashing where the bundle is null.
> > Null checking that stops the crash but leaves a blank option in the device
> > selection menu which will crash if I select it. I'll dig in further, but
> > just wanted to note that this shouldn't land as-is.
> 
> I'm surprised at this and I'd like to know why its happening rather than
> wallpaper over it. Are you using an updated play services? Is this a real
> chromecast or a firefly? Is your Chromecast up to date?

It's the firefly. We can't crash on startup because of a bad discover response though.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #92)
> (In reply to Wesley Johnston (:wesj) from comment #86)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #77)
> > > Comment on attachment 8439581 [details] [diff] [review]
> > > Patch 2/3 - Chromecast MediaPlayer
> > > 
> > > Review of attachment 8439581 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This code as-is crashes Fennec on start up. selectedDevice is null in
> > > toJSON(). Null checking that gets me to crashing where the bundle is null.
> > > Null checking that stops the crash but leaves a blank option in the device
> > > selection menu which will crash if I select it. I'll dig in further, but
> > > just wanted to note that this shouldn't land as-is.
> > 
> > I'm surprised at this and I'd like to know why its happening rather than
> > wallpaper over it. Are you using an updated play services? Is this a real
> > chromecast or a firefly? Is your Chromecast up to date?
> 
> It's the firefly. We can't crash on startup because of a bad discover
> response though.

Useful though. Thanks. I think maybe we want to handle the Firefly in JS anyway, so maybe we're best to throw away results if this fails.
root cause is 'CastDevice.getFromBundle(route.getExtras());' returns null. So I created a static createChromeCast function in Chromecast.java: 

    public static ChromeCast createChromeCast(Context context, RouteInfo route) {
        CastDevice device = CastDevice.getFromBundle(route.getExtras());
        if (device == null)
            return null;
        return new ChromeCast(context, route, device);
    }

and only add it to displays in MediaPlayerManager.java if that returns non-null
Attached patch Patch 3/3 (obsolete) — Splinter Review
I think this addresses the comments.
Attachment #8439583 - Attachment is obsolete: true
Attachment #8444267 - Flags: review?(mark.finkle)
Attached patch Patch 2/3Splinter Review
This is a bit different than Brad's patch. I need to test it, but it basically should throw in device.toJSON(). That should be caught now so that we don't crash or send the null result to JS. Need to test.
Attachment #8439581 - Attachment is obsolete: true
Attachment #8444268 - Flags: review?(mark.finkle)
Attached patch Patch 1/3Splinter Review
Attachment #8439579 - Attachment is obsolete: true
Attachment #8444269 - Flags: review+
Comment on attachment 8444267 [details] [diff] [review]
Patch 3/3

>diff --git a/mobile/android/chrome/content/CastingApps.js b/mobile/android/chrome/content/CastingApps.js

>+var mediaPlayerTarget = {

>+    Cu.import("resource://gre/modules/MediaPlayerApp.jsm");
>+    return new MediaPlayerApp(aService, "MediaRouter");

"MedaRouter" arg is unused. You can remove it.

>     // Register targets
>     SimpleServiceDiscovery.registerTarget(rokuTarget);
>     SimpleServiceDiscovery.registerTarget(fireflyTarget);
>-
>+    SimpleServiceDiscovery.registerTarget(mediaPlayerTarget);
>     // Search for devices continuously every 120 seconds
>     SimpleServiceDiscovery.search(120 * 1000);

I like my whitespace. Please keep the blank line before the comment

>diff --git a/mobile/android/modules/MediaPlayerApp.jsm b/mobile/android/modules/MediaPlayerApp.jsm


>+function log(msg) {
>+  Services.console.logStringMessage(msg);
>+}

If you're going to keep the log method in the file, try moving to the AndroidLog approach:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/SimpleServiceDiscovery.jsm#17

>+function send(type, data, callback) {
>+  var msg = {

let

>+  for (var i in data) {

let

>+  start: function start(callback) {
>+    send("MediaPlayer:Start", {id: this.id}, (result) => {

Add spaces inside the { }
{ id: this.id }

>diff --git a/mobile/android/modules/SimpleServiceDiscovery.jsm b/mobile/android/modules/SimpleServiceDiscovery.jsm

>+    // We also query Java directly here for any devices that Android might support natively (i.e. Chromecast or Miracast)
>+    sendMessageToJava({ type: "MediaPlayer:Get" }, (result) => {

You didn't move this code into it's own method. Any reason why not?

>+        // Convert the native data into something matching what is created in _processService()
>+        let service = {

The comment says we'll pass this to _processService, but we never do. Should we? processService is used to filter some targets, which will never happen as is.

I don't think we can call _processService since it does an XHR to get the meta data and we can't do that hear. You should consider using the filterService call in here though:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/SimpleServiceDiscovery.jsm#317

>+        // Only add and notify if we don't already know about this service
>+        if (!this._services.has(service.location)) {
>+          this._services.set(service.location, service);

We don't use "location" as the key anymore. We use "uuid" now.

>+          Services.obs.notifyObservers(null, EVENT_SERVICE_FOUND, service.location);

We pass service.uuid as the last param too.

>+        }
>+
>+        // Make sure we remember this service is not stale
>+        this._services.get(service.location).lastPing = this._searchTimestamp;

Use service.uuid as the key 

r- for the filterService and service.uuid changes
Attachment #8444267 - Flags: review?(mark.finkle) → review-
Attachment #8444268 - Flags: review?(mark.finkle) → review+
Fixed up.
Attachment #8444267 - Attachment is obsolete: true
Attachment #8445561 - Flags: review?(mark.finkle)
Attachment #8445561 - Flags: review?(mark.finkle) → review+
Attached patch FixupSplinter Review
Went to land this and found a mistake. This defines MOZ_NATIVE_DEVICES unconditionally for the preprocessor. We don't want that. Fixing it unveils some issues with the initialization at startup. i.e. we try not to compile the MediaPlayerManager, but since we can't preprocess BrowserApp the class has to have a definition there. Instead, I used reflection here. I'm not sure of any better way to do this (maybe we compile a stub if this is turned off?)

Seemed like a big enough change to run by before I land.
Attachment #8448470 - Flags: review?(mark.finkle)
Comment on attachment 8448470 [details] [diff] [review]
Fixup

Review of attachment 8448470 [details] [diff] [review]:
-----------------------------------------------------------------

wfm.  I noticed the funky define, but didn't realize it was actually wrong.

::: mobile/android/base/BrowserApp.java
@@ +555,3 @@
>          if (AppConstants.MOZ_MEDIA_PLAYER) {
> +            try {
> +                Class<?> mediaManagerClass = Class.forName("org.mozilla.gecko.MediaPlayerManager");

Factor this out.  You might add |getMediaPlayerManager| which has the AppConstants check in it, and null check the result.

::: mobile/android/base/moz.build
@@ +527,5 @@
>      'res/raw/suggestedsites.json',
>      'res/values/strings.xml',
>  ]
>  
> +for var in ('MOZ_ANDROID_ANR_REPORTER', 'MOZ_LINKER_EXTRACT', 'MOZILLA_OFFICIAL', 'MOZ_DEBUG', 'MOZ_NATIVE_DEVICES'):

Rebase error?  You lost search activity.
Attachment #8448470 - Flags: review?(mark.finkle) → review+
Has the implementation of this landed? If so, it's not obvious how to cast a tab.
(In reply to Paul [pwd] from comment #104)
> Has the implementation of this landed? If so, it's not obvious how to cast a
> tab.

The implementation has landed and can be enabled by local developer builds. We are working on adding Play Services support libraries to the production builders (bug 1016529), which will allow us to enable the feature for Nightly and other channels.
QA Contact: aaron.train
Depends on: 1046536
Depends on: 1046537
Depends on: 1042238
Attachment #8440160 - Flags: feedback?(wjohnston)
Depends on: 1057487
Depends on: 1057866
Depends on: 1056054
Depends on: 1053889
Depends on: 1040958
Depends on: 1055020
Depends on: 1055553
Depends on: 1055554
Depends on: 1055764
Depends on: 1061032
You need to log in before you can comment on or make changes to this bug.