Closed Bug 953381 Opened 6 years ago Closed 6 years ago

Add generic support for casting a video to a second screen service

Categories

(Firefox for Android :: General, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: mfinkle, Assigned: mfinkle)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

Part of the work for bug 921924 is finding a suitable <video> element on a web page, sending the content to a second screen service and showing a UI in Firefox to control the playback of the video on the second screen.
Attached patch Find and cast video v0.1 (obsolete) — Splinter Review
This patch does a few things on the Gecko side:
* Adds CastApps object to encapsulate the behavior of finding and casting a video
* Adds a context menu for casting a <video>, if we think you have long pressed on a suitable <video>
  * A suitable <video> has a MP4 source
  * We look at the element where you long pressed as a candidate <video>.
  * Since the page could be using a cover image, we fallback to scanning for <video>s in the same x, y location as the long press.

* CastApps uses SimpleServiceDiscovery to look for services, and apps on those services, where we could send a video.
  * No casting Apps are registered in this patch. That's in bug 921948.
* Assuming an App is found, we fill in a video title and poster, and start a remote media session with the App.
  * A remote media session creates a protocol connection from Firefox to the App so we can control playback and get status. See bug 921948 for an example.
Attachment #8351670 - Flags: feedback?(wjohnston)
Attached patch Casting control UI v.01 (obsolete) — Splinter Review
This patch creates an Android UI to control playback, once a remote media session has been created/started.

I based this UI from the Findbar code. The UI remains visible as long as a remote media session is active. If you navigate to a new page or switch tabs, the control UI remains active.
Attachment #8351671 - Flags: feedback?(wjohnston)
Attached patch Find and cast video v0.2 (obsolete) — Splinter Review
Found a typo in the v0.1 version
Assignee: nobody → mark.finkle
Attachment #8351670 - Attachment is obsolete: true
Attachment #8351670 - Flags: feedback?(wjohnston)
Attachment #8354954 - Flags: feedback?(wjohnston)
These are a few tests for the CastApps.getVideo functionality (in Find and cast video patch). It uses the same basic framework I used in the search and feed discovery tests (bug 952894).

CastApps.getVideo is the core functionality, so some tests are useful. The tests include some basic <video> elements as well as some expected fail cases too. We must find an MP4 video. I also add a simple "div overlay" example. Some websites use this approach with their videos.
Attachment #8354955 - Flags: feedback?(wjohnston)
Comment on attachment 8354954 [details] [diff] [review]
Find and cast video v0.2

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

Seems like good ideas. Some comments on the API's that probably go in a different patch.

::: mobile/android/chrome/content/browser.js
@@ +8053,5 @@
> +
> +  getVideo: function(aElement, aX, aY) {
> +    dump("XXX start looking for a video")
> +    // Fast path: Is the given element a video element
> +    let source = this._getVideo(aElement);

This is more than just the source. We usually call it "video" in other places.

@@ +8063,5 @@
> +    // to find an element we match. When it hits <html> things can go BOOM.
> +    try {
> +      // Maybe this is an overlay, with the video element under it
> +      // Use the (x, y) location to guess at a <video> element
> +      let videos = aElement.ownerDocument.querySelectorAll("video");

I wonder if you'd be better to use nodesFromRect: https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIDOMWindowUtils#nodesFromRect%28%29

and then filter that list for videos. Maybe context menus should do that... but its likely to cause reflows. getBoundingClientRect will also probably do that.

@@ +8105,5 @@
> +      let sourceURI = this.makeURI(sourceURL, null, this.makeURI(aElement.baseURI));
> +      if (sourceURI && sourceURI instanceof Ci.nsIURL) {
> +        let extension = sourceURI.fileExtension;
> +        dump("XXX: found extension: " + extension)
> +        if (extension == "mp4") {

This should probably be a pref... or something the app specifies... or both. App.canOpen(url, mimetype)?

@@ +8123,5 @@
> +        dump("XXX: found source: " + sourceURI.spec)
> +        return { video: aElement, source: sourceURI.spec, poster: posterURL };
> +      }
> +
> +      // Fallback to using the file extension to guess the mime type

We could also kick off a request and see what the return mimetype was (then immediately kill the request).

@@ +8129,5 @@
> +        let extension = sourceURI.fileExtension;
> +        dump("XXX: found source extension: " + extension)
> +        if (extension == "mp4") {
> +          return { video: aElement, source: sourceURI.spec, poster: posterURL };
> +        }

Probably could move this repeated code into a helper.

@@ +8149,5 @@
> +      return (video != null);
> +    }
> +  },
> +
> +  prompt: function(aCallback) {

I'd love to skip this if there's only one service.

@@ +8209,5 @@
> +        dump("XXX: " + video.poster);
> +
> +        app.stop(function() {
> +          app.start("", function() {
> +            app.remoteMedia(function(aRemoteMedia) {

I need to look at the other patches, but do we need this separate remote media request? Can we get rid of that object and make this whole thing just a simpler

app.start(url, () => { });

@@ +8212,5 @@
> +          app.start("", function() {
> +            app.remoteMedia(function(aRemoteMedia) {
> +              dump("XXX starting remote media")
> +              this.session = {
> +                service: aService,

These service/app/removeMedia are confusing to me, but it looks like we never use service. Can we drop it?

@@ +8243,5 @@
> +  onRemoteMediaStart: function(aRemoteMedia) {
> +    dump("XXX remote media ready")
> +    if (this.session) {
> +      dump("XXX remote media load");
> +      this.session.remoteMedia.load(this.session.data);

aRemoteMedia?

@@ +8254,5 @@
> +
> +  onRemoteMediaStatus: function(aRemoteMedia) {
> +    dump("XXX remote media status")
> +    if (this.session) {
> +      let status = this.session.remoteMedia.status;

aRemoteMedia?

::: mobile/android/locales/en-US/chrome/browser.properties
@@ +159,5 @@
>  # Text Selection
>  selectionHelper.textCopied=Text copied to clipboard
>  
> +# Casting
> +casting.prompt=Cast to Device

One string is "Cast to device" the other "Cast to screen"?
Attachment #8354954 - Flags: feedback?(wjohnston) → feedback+
If we NEED the prompt to show the device, you can pass a function to the NativeWindow.contextmenu method for the title, and then we should just show "Cast to Roku" there instead of a generic "Cast to screen".
Comment on attachment 8354955 [details] [diff] [review]
Video discovery tests v0.1

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

Nice!

::: mobile/android/base/tests/testVideoDiscovery.js
@@ +37,5 @@
> +  let url = "http://mochi.test:8888/tests/robocop/video_discovery.html";
> +  browser = BrowserApp.addTab(url, { selected: true, parentId: BrowserApp.selectedTab.id }).browser;
> +  browser.addEventListener("load", function startTests(event) {
> +    browser.removeEventListener("load", startTests, true);
> +    Services.tm.mainThread.dispatch(run_next_test, Ci.nsIThread.DISPATCH_NORMAL);

Why do you need this and not just run_next_test();

@@ +56,5 @@
> +  if (element) {
> +    let [x, y] = middle(element);
> +    let video = chromeWin.CastApps.getVideo(element, x, y);
> +    if (video) {
> +      let matchPoster = (test.poster == video.poster);

Why not makes these individual tests?

@@ +69,5 @@
> +  run_next_test();
> +}
> +
> +let videoTest;
> +while ((videoTest = videoDiscoveryTests.shift())) {

Why are you doing this and not a simpler loop?

@@ +71,5 @@
> +
> +let videoTest;
> +while ((videoTest = videoDiscoveryTests.shift())) {
> +  if (!("poster" in videoTest)) {
> +    videoTest.poster = "";

I'd prefer something like videoTest.poster = videoTest.poster || "";
Comment on attachment 8351671 [details] [diff] [review]
Casting control UI v.01

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

I like this :)

::: mobile/android/base/MediaCastingBar.java
@@ +47,5 @@
> +        mMediaPlay.setOnClickListener(this);
> +        mMediaPause = (ImageButton) content.findViewById(R.id.media_pause);
> +        mMediaPause.setOnClickListener(this);
> +        mMediaStop = (ImageButton) content.findViewById(R.id.media_stop);
> +        mMediaStop.setOnClickListener(this);

Is this a "Stop" or a "Close" button? If its close, I'd rather name every thing that way.

@@ +102,5 @@
> +            @Override
> +            public void run() {
> +                if (event.equals("Casting:Started")) {
> +                    show();
> +                    if (TextUtils.isEmpty(device) == false) {

if (!TextUtils.isEmpty(device)) {

::: mobile/android/chrome/content/browser.js
@@ +8043,5 @@
>      );
> +
> +    Services.obs.addObserver(this, "Casting:Play", false);
> +    Services.obs.addObserver(this, "Casting:Pause", false);
> +    Services.obs.addObserver(this, "Casting:Stop", false);

We should move this whole thing into a separate file and lazy load it. The context menu makes that hard. I want to fix! But we can still isolate it into its own file and non-lazy load for now...
Attachment #8351671 - Flags: feedback?(wjohnston) → feedback+
(In reply to Wesley Johnston (:wesj) from comment #7)

> ::: mobile/android/base/tests/testVideoDiscovery.js

Note: I used the same system (borrowed from some desktop tests) in bug 952894. See here for review comments on that:
https://bugzilla.mozilla.org/show_bug.cgi?id=952894#c3

> > +  browser.addEventListener("load", function startTests(event) {
> > +    browser.removeEventListener("load", startTests, true);
> > +    Services.tm.mainThread.dispatch(run_next_test, Ci.nsIThread.DISPATCH_NORMAL);
> 
> Why do you need this and not just run_next_test();

In the code I stole the idea from, do_execute_soon was used. I think we want to let the event handler finish before passing to the next test.

> 
> > +  if (element) {
> > +    let [x, y] = middle(element);
> > +    let video = chromeWin.CastApps.getVideo(element, x, y);
> > +    if (video) {
> > +      let matchPoster = (test.poster == video.poster);
> 
> Why not makes these individual tests?

They are created into individual tests. But since all of the code is repeated, we put the data in an array of objects and just bind to these test functions.

> > +  run_next_test();
> > +}
> > +
> > +let videoTest;
> > +while ((videoTest = videoDiscoveryTests.shift())) {
> 
> Why are you doing this and not a simpler loop?

It's the way I did it in bug 952894.

> > +  if (!("poster" in videoTest)) {
> > +    videoTest.poster = "";
> 
> I'd prefer something like videoTest.poster = videoTest.poster || "";

I was asked to change to this format in bug 952894.
(In reply to Wesley Johnston (:wesj) from comment #5)

> > +  getVideo: function(aElement, aX, aY) {
> > +    dump("XXX start looking for a video")
> > +    // Fast path: Is the given element a video element
> > +    let source = this._getVideo(aElement);
> 
> This is more than just the source. We usually call it "video" in other
> places.

Done

> > +    // to find an element we match. When it hits <html> things can go BOOM.
> > +    try {
> > +      // Maybe this is an overlay, with the video element under it
> > +      // Use the (x, y) location to guess at a <video> element
> > +      let videos = aElement.ownerDocument.querySelectorAll("video");
> 
> I wonder if you'd be better to use nodesFromRect:
> https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/
> nsIDOMWindowUtils#nodesFromRect%28%29
> 
> and then filter that list for videos. Maybe context menus should do that...
> but its likely to cause reflows. getBoundingClientRect will also probably do
> that.

It might be better to use nodesFromRect. I'd like to see if we even want to keep this fallback approach before doing anything more to it. We might take the approach that <video>s need to be playing before we can cast them. We just need to use it a bit and see how it works and if it works on enough sites.

> > +        if (extension == "mp4") {
> 
> This should probably be a pref... or something the app specifies... or both.
> App.canOpen(url, mimetype)?

We don't have an app yet. I agree that we should see how this evolves and make some changes. I don't want to add any complexity now though.

> > +      // Fallback to using the file extension to guess the mime type
> 
> We could also kick off a request and see what the return mimetype was (then
> immediately kill the request).

Maybe, but that might be overkill for just checking on the context menu. This code is used when checking on whether to show the context menu, so I want to favor being fast. Again, if we decide to wait for the <video> to start playing, we might have a better way to ask the <video> for the mime type.

> > +        let extension = sourceURI.fileExtension;
> > +        dump("XXX: found source extension: " + extension)
> > +        if (extension == "mp4") {
> > +          return { video: aElement, source: sourceURI.spec, poster: posterURL };
> > +        }
> 
> Probably could move this repeated code into a helper.

Done

> > +  prompt: function(aCallback) {
> 
> I'd love to skip this if there's only one service.

I am torn on that. I like some UX consistency with menus/actions.

> > +        app.stop(function() {
> > +          app.start("", function() {
> > +            app.remoteMedia(function(aRemoteMedia) {
> 
> I need to look at the other patches, but do we need this separate remote
> media request? Can we get rid of that object and make this whole thing just
> a simpler
> 
> app.start(url, () => { });

The "app" is not a simple wrapper around some query/launch APIs on the device (Roku and Chromecast). The "remoteMedia" is a server (TCP or WebSocket) connection between Firefox and the device. I definitely want to wrap, and control, them separately. We could look at making app.start also start the remoteMedia connection.

Followup

> > +          app.start("", function() {
> > +            app.remoteMedia(function(aRemoteMedia) {
> > +              dump("XXX starting remote media")
> > +              this.session = {
> > +                service: aService,
> 
> These service/app/removeMedia are confusing to me, but it looks like we
> never use service. Can we drop it?

Service is the Roku, App is the Firefox App on the Roku, RemoteMedia is the live connection from the App to Fennec.

We use session.service in a later patch, but I can remove session.remoteMedia based on your suggestions below.

> > +  onRemoteMediaStart: function(aRemoteMedia) {

> > +      this.session.remoteMedia.load(this.session.data);
> 
> aRemoteMedia?

> > +  onRemoteMediaStatus: function(aRemoteMedia) {

> > +      let status = this.session.remoteMedia.status;
> 
> aRemoteMedia?

Done

> ::: mobile/android/locales/en-US/chrome/browser.properties

> > +casting.prompt=Cast to Device
> 
> One string is "Cast to device" the other "Cast to screen"?

I'll let UX help with this one as we move ahead:
"Cast to Screen" is the context menu. Using "device" seems weird there since a dive could be anything.
"Cast to Device" is the title of the prompt asking you "which device should we cast this to?" Using "screen" there seems weird, since this is a list of devices hooked to screens.
(In reply to Mark Finkle (:mfinkle) from comment #9)
> > Why not makes these individual tests?
> 
> They are created into individual tests. But since all of the code is
> repeated, we put the data in an array of objects and just bind to these test
> functions.

I think we're talking about different things. This has code that looks like:
var test1 = test.a === video.a;
var test2 = test.b === video.b;
ok(test1 && test2, "Test passed");

instead of:
is(test.a, video.a, "A matches");
is(test.b, video.b, "B matches");
(In reply to Wesley Johnston (:wesj) from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > > Why not makes these individual tests?
> > 
> > They are created into individual tests. But since all of the code is
> > repeated, we put the data in an array of objects and just bind to these test
> > functions.
> 
> I think we're talking about different things. This has code that looks like:
> var test1 = test.a === video.a;
> var test2 = test.b === video.b;
> ok(test1 && test2, "Test passed");
> 
> instead of:
> is(test.a, video.a, "A matches");
> is(test.b, video.b, "B matches");

Ah. Your comment makes total sense now. I kept it a single test because I wanted each test array item to be a single test. It certainly could be broken out separately, but I didn't see the value. Fail is a fail.
Updated based on some of the feedback. In addition, I am moving CastApps into it's own JS file and removing the dump statements in a new patch.
Attachment #8354954 - Attachment is obsolete: true
Attachment #8359607 - Flags: review?(wjohnston)
Comment on attachment 8354955 [details] [diff] [review]
Video discovery tests v0.1

Changing request from feedback to review.
Attachment #8354955 - Flags: feedback?(wjohnston) → review?(wjohnston)
This patch moves the CastApp object from browser.js to CastingApps.js. It also removes all the dump calls. It also adds CastingApps.session.remoteMedia back because it's used in the Control UI patch.
Attachment #8359610 - Flags: review?(wjohnston)
Updated the patch with the feedback. I also tweaked the layout XML a little bit. UX wanted the Play button centered, not right next to the stop/disconnect button.

I expect some additional tweaks to the UI, but that can be a different bug.
Attachment #8351671 - Attachment is obsolete: true
Attachment #8359616 - Flags: review?(wjohnston)
Comment on attachment 8359616 [details] [diff] [review]
Casting control UI v.02

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

I wonder if it would be better to name these images media_bar_stop.png (instead of media_stop.png). We show play/pause in other places, so this naming could lead to confusion.

::: mobile/android/base/MediaCastingBar.java
@@ +21,5 @@
> +
> +public class MediaCastingBar extends RelativeLayout implements View.OnClickListener, GeckoEventListener  {
> +    private static final String LOGTAG = "MediaCastingBar";
> +
> +    private final Context mContext;

The relative layout will store a reference to this for you that you can get via. getContext(). Unless we need to remove the tiny overhead of that call, we don't need this.

@@ +105,5 @@
> +                    show();
> +                    if (!TextUtils.isEmpty(device)) {
> +                        mCastingTo.setText(device);
> +                    } else {
> +                        mCastingTo.setText("Unknown device");

Localize. Also, I hate this string. I'd rather we just left it blank.

@@ +108,5 @@
> +                    } else {
> +                        mCastingTo.setText("Unknown device");
> +                    }
> +                    mMediaPlay.setVisibility(GONE);
> +                    mMediaPause.setVisibility(VISIBLE);

Are we sure this will always start in play mode? I think we are but just checking :)

::: mobile/android/base/resources/layout/media_casting.xml
@@ +4,5 @@
> +    <RelativeLayout android:id="@+id/media_controls"
> +                    android:layout_width="wrap_content"
> +                    android:layout_height="wrap_content"
> +                    android:layout_centerInParent="true">
> +                    

ws

@@ +24,5 @@
> +              android:layout_height="wrap_content"
> +              android:layout_marginLeft="5dip"
> +              android:layout_marginRight="5dip"
> +              android:layout_alignParentLeft="true"
> +              android:layout_toLeftOf="@id/media_controls"

alignParentLeft and toLeftOf are mutually exclusive. You only need the second. I would (personally) rather we made this whole bar a LinearLayout and made this guy "flex" (I wonder what happens right now if your string is very long....). LinearLayout is nice if we ever want to add more (Next and prev in the queue?). I have a feeling that calculating layouts for a LinearLayout are slightly more expensive than Relative though, so I bet you could make a fine argument for this if you want :)
Comment on attachment 8359616 [details] [diff] [review]
Casting control UI v.02

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

r+ with the localization fix.
Attachment #8359616 - Flags: review?(wjohnston) → review+
Comment on attachment 8359610 [details] [diff] [review]
Move CastingApps to separate JS file v0.1

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

::: mobile/android/chrome/content/CastingApps.js
@@ +7,5 @@
> +  _castMenuId: -1,
> +
> +  init: function ca_init() {
> +    // Search for devices continuously every 120 seconds
> +    SimpleServiceDiscovery.search(120000);

Nit: My personal preference is to prefer writing out the math for these 120*1000.

@@ +32,5 @@
> +      return video;
> +    }
> +
> +    // The context menu system will keep walking up the DOM giving us a chance
> +    // to find an element we match. When it hits <html> things can go BOOM.

Hmm... Good point. If there is a video, this is fine. If there is none, we'll do this over and over. Not sure how to avoid that... We should at least file a bug to look into caching this somehow.

@@ +86,5 @@
> +      }
> +    }
> +
> +    // Next, look to see if there is a <source> child element that meets
> +    // our needs

wrapping? When did you start wrapping lines

@@ +96,5 @@
> +      if (sourceNode.type == "video/mp4") {
> +        return { video: aElement, source: sourceURI.spec, poster: posterURL };
> +      }
> +
> +      // Fallback to using the file extension to guess the mime type

I would rather combine this with the if above. if (sourceNode.type || allowableExtension) { }

@@ +140,5 @@
> +
> +  openExternal: function(aElement, aX, aY) {
> +    // Start a second screen media service
> +    let video = this.getVideo(aElement, aX, aY);
> +    if (video) {

if (!video)
  return;

@@ +150,5 @@
> +        let app = SimpleServiceDiscovery.findAppForService(aService, "video-sharing");
> +        if (!app)
> +          return;
> +
> +        video.title = aElement.ownerDocument.defaultView.top.document.title;

aElement.title might be useful here...

@@ +189,5 @@
> +  onRemoteMediaStop: function(aRemoteMedia) {
> +  },
> +
> +  onRemoteMediaStatus: function(aRemoteMedia) {
> +    if (this.session) {

if (!this.session)
  return;

@@ +193,5 @@
> +    if (this.session) {
> +      let status = aRemoteMedia.status;
> +      if (status == "completed") {
> +        aRemoteMedia.shutdown();
> +        this.session.app.stop();

If we do eventually support queues, this may not want to shutdown so completely?
Attachment #8359610 - Flags: review?(wjohnston) → review+
Comment on attachment 8359607 [details] [diff] [review]
Find and cast video v0.3

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

I reviewed this code in the other patch (going backwards today it seems). It looks the same/cleaned up from this, so I'm just r+'ing. The strings question still remains, but I see your point. They're slightly different contexts....
Attachment #8359607 - Flags: review?(wjohnston) → review+
Comment on attachment 8354955 [details] [diff] [review]
Video discovery tests v0.1

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

Not my favorite code :) But for tests, I can live with things. Especially since, "Yay more tests!"
Attachment #8354955 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #17)

> I wonder if it would be better to name these images media_bar_stop.png
> (instead of media_stop.png). We show play/pause in other places, so this
> naming could lead to confusion.

Done

> > +    private final Context mContext;
> 
> The relative layout will store a reference to this for you that you can get
> via. getContext(). Unless we need to remove the tiny overhead of that call,
> we don't need this.

Done

> > +                        mCastingTo.setText("Unknown device");
> 
> Localize. Also, I hate this string. I'd rather we just left it blank.

Done. It's blank for now.

> > +                    mMediaPlay.setVisibility(GONE);
> > +                    mMediaPause.setVisibility(VISIBLE);
> 
> Are we sure this will always start in play mode? I think we are but just
> checking :)

We are sure. We should change that in the future. We should be able to re-connect to an ongoing session. But not yet.

> > +                    
> 
> ws

Fixed

> > +              android:layout_alignParentLeft="true"
> > +              android:layout_toLeftOf="@id/media_controls"
> 
> alignParentLeft and toLeftOf are mutually exclusive. You only need the
> second. I would (personally) rather we made this whole bar a LinearLayout
> and made this guy "flex" (I wonder what happens right now if your string is
> very long....). LinearLayout is nice if we ever want to add more (Next and
> prev in the queue?). I have a feeling that calculating layouts for a
> LinearLayout are slightly more expensive than Relative though, so I bet you
> could make a fine argument for this if you want :)

I left these for now. alignParentLeft will keep the left edge of the TextView aligned to the left edge of the parent. toLeftOf will keep the right edge of the TextView aligned to the left edge of the Play/Pause control.

UX designs have the Play/Pause control centered, which is why a LinearLayout was failing to work for me. I had to switch (it was a LinearLayout) to a RelativeLayout to get the centered control. We can try to improve this layout as we add controls.
(In reply to Wesley Johnston (:wesj) from comment #19)

> > +    // Search for devices continuously every 120 seconds
> > +    SimpleServiceDiscovery.search(120000);
> 
> Nit: My personal preference is to prefer writing out the math for these
> 120*1000.

Done

> > +    // Next, look to see if there is a <source> child element that meets
> > +    // our needs
> 
> wrapping? When did you start wrapping lines

I'm OK with wrapping comments. It's breaking lines of code in weird ways that irks me.

> > +      if (sourceNode.type == "video/mp4") {
> > +        return { video: aElement, source: sourceURI.spec, poster: posterURL };
> > +      }
> > +
> > +      // Fallback to using the file extension to guess the mime type
> 
> I would rather combine this with the if above. if (sourceNode.type ||
> allowableExtension) { }

Done

> > +    if (video) {
> 
> if (!video)
>   return;

Done

> > +  onRemoteMediaStatus: function(aRemoteMedia) {
> > +    if (this.session) {
> 
> if (!this.session)
>   return;

Done

> > +      if (status == "completed") {
> > +        aRemoteMedia.shutdown();
> > +        this.session.app.stop();
> 
> If we do eventually support queues, this may not want to shutdown so
> completely?

You might be right. Depends on how we implement the queue.
You need to log in before you can comment on or make changes to this bug.