Closed Bug 1001280 Opened 6 years ago Closed 6 years ago

Add a pageaction for casting videos

Categories

(Firefox for Android :: Screencasting, defect)

x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 32
Tracking Status
firefox31 --- verified
firefox32 --- verified

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch casting-pageaction v0.1 (obsolete) — Splinter Review
We already support casting via long-tap menu and the video element controls, but both of those UIs can be overridden by the web page. This can make it difficult to start casting a video.

This patch adds support for a pageaction UI to start casting a video. The only caveat is that the video must be the right format (MP4 currently) and it must be playing. We use the "playing" part to trigger which video the pageaction will start casting. If two videos are playing, whichever one was started last is the video controlled by the pageaction.

The pageaction should display a "ready to cast" icon when a suitable video is playing. The pageaction should change to a "actively casting" icon when the video has been sent to a second screen by any means (long tap or video controls), not just the pageaction.

The pageaction should hide when switching tabs or loading new pages, unless a suitable video is playing or being cast on the new page.

When the video is done playing or done being cast, the pageaction should also hide.
Screenshot of the "ready to cast" icon
Assignee: nobody → mark.finkle
Screenshot of the "actively casting" icon
Attachment #8412369 - Flags: review?(wjohnston)
Comment on attachment 8412369 [details] [diff] [review]
casting-pageaction v0.1

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

I want to be a little more careful about what video is casting here. I'm curious what happens if you "cast" from two different tabs at once too...

::: mobile/android/chrome/content/CastingApps.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  "use strict";
>  
>  var CastingApps = {
> +  SCAN_VIDEO: null, // flag to do a full document scan for a castable <video>

I'd rather just not pass anything than use this. We can make aVideo default to null in the updatePageAction method.

@@ +133,5 @@
>      // Start the new session
>      this.openExternal(video, 0, 0);
>    },
>  
> +  handleVideoCheck: function handleVideoCheck(aTab, aEvent) {

I hate this naming. Can we just move all this into updatePageAction(null, aTab)? updatePageActionForTab()

@@ +144,5 @@
> +    // Update the page action, scanning for a castable <video>
> +    this._updatePageAction(this.SCAN_VIDEO);
> +  },
> +
> +  handleVideoPlaying: function handleVideoPlaying(aEvent) {

This name also confuses me. I'd also be tempted to just put this in updatePageAction.

@@ +151,5 @@
> +      return;
> +    }
> +
> +    // If playing, send the <video>, but if ended we send nothing to shutdown the pageaction
> +    this._updatePageAction(aEvent.type == "playing" ? video : this.SCAN_VIDEO);

This will fire for every video on the page, not just the one that's being cast...

@@ +279,5 @@
> +    // a castable video
> +    if (this.pageAction.id) {
> +      NativeWindow.pageactions.remove(this.pageAction.id);
> +      delete this.pageAction.id;
> +    }

I worry a little about thrashing the API with this. i.e. if for some reason updatePageAction is called twice on the same tab, we'll delete it and recreate it quickly. We could move this down below and only delete what we have if we're changing state (no video to have video, detecting whether we're moving from casting to not-casting is harder without saving more info).

@@ +281,5 @@
> +      NativeWindow.pageactions.remove(this.pageAction.id);
> +      delete this.pageAction.id;
> +    }
> +
> +    if (!aVideo) {

If the play/stop api is going to call in here, I guess we need to do this regardless and search for any other videos that might be casting. That feels a bit awful. Ideally, we'd keep a WeakRef to the casting video on the tab itself or something...

@@ +291,5 @@
> +        if (video.paused && unwrappedVideo.mozIsCasting) {
> +          // This <video> is cast-active. Break out of loop.
> +          aVideo = video;
> +          break;
> +        }

add empty line.

@@ +292,5 @@
> +          // This <video> is cast-active. Break out of loop.
> +          aVideo = video;
> +          break;
> +        }
> +        if (!video.paused && unwrappedVideo.mozAllowCasting && !castVideo) {

Too many patches around, but what is castVideo. Should this be aVideo?

@@ +294,5 @@
> +          break;
> +        }
> +        if (!video.paused && unwrappedVideo.mozAllowCasting && !castVideo) {
> +          // This <video> is cast-ready. Keep looking so cast-active could be found.
> +          aVideo = video;

I wonder if you need to check sources/airplay tags here.

@@ +300,5 @@
> +      }
> +
> +      // The scan turned up no castable <video>
> +      if (!aVideo) {
> +        return;

Can you pull this video element finder stuff into its own helper function?
Attachment #8412369 - Flags: review?(wjohnston) → review-
(In reply to Wesley Johnston (:wesj) from comment #3)

> I want to be a little more careful about what video is casting here. I'm
> curious what happens if you "cast" from two different tabs at once too...

We can currently only cast one video at a time, so you can't cast from two different tabs at once.

> ::: mobile/android/chrome/content/CastingApps.js
> @@ +3,5 @@
> >   * You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  "use strict";
> >  
> >  var CastingApps = {
> > +  SCAN_VIDEO: null, // flag to do a full document scan for a castable <video>
> 
> I'd rather just not pass anything than use this. We can make aVideo default
> to null in the updatePageAction method.

I can do that. I thought the SCAN_VIDEO was a bit more explicit and told you exactly what updatePageAction would do when SCAN_VIDEO is passed. Passing no second parameter looks cleaner, but doesn't tell you what's going to happen.

> >      // Start the new session
> >      this.openExternal(video, 0, 0);
> >    },
> >  
> > +  handleVideoCheck: function handleVideoCheck(aTab, aEvent) {
> 
> I hate this naming. Can we just move all this into updatePageAction(null,
> aTab)? updatePageActionForTab()

I want to leave it for now. We might be able to create a updatePageActionForTab, which just calls updatePageAction. But I like that these methods are event handlers, and you can tell from the naming.

> > +  handleVideoPlaying: function handleVideoPlaying(aEvent) {
> 
> This name also confuses me. I'd also be tempted to just put this in
> updatePageAction.

You can't put all of this in updatePageAction

> > +    // If playing, send the <video>, but if ended we send nothing to shutdown the pageaction
> > +    this._updatePageAction(aEvent.type == "playing" ? video : this.SCAN_VIDEO);
> 
> This will fire for every video on the page, not just the one that's being
> cast...

Really? Why? handleVideoPlaying should only be called for videos that start or finish playing. What am I missing. (hint: if you know a answer, don't keep it a secret in the review comment)

> > +    // a castable video
> > +    if (this.pageAction.id) {
> > +      NativeWindow.pageactions.remove(this.pageAction.id);
> > +      delete this.pageAction.id;
> > +    }
> 
> I worry a little about thrashing the API with this. i.e. if for some reason
> updatePageAction is called twice on the same tab, we'll delete it and
> recreate it quickly. We could move this down below and only delete what we
> have if we're changing state (no video to have video, detecting whether
> we're moving from casting to not-casting is harder without saving more info).

OK. I can look into making it a bit cleaner. I copied the pattern from Reader in browser.js

> > +      NativeWindow.pageactions.remove(this.pageAction.id);
> > +      delete this.pageAction.id;
> > +    }
> > +
> > +    if (!aVideo) {
> 
> If the play/stop api is going to call in here, I guess we need to do this
> regardless and search for any other videos that might be casting. That feels
> a bit awful. Ideally, we'd keep a WeakRef to the casting video on the tab
> itself or something...

We do keep a WeakRef to the casting video. But we need to handle more cases than that. We mainly do the scan to look for videos that are not being cast, but could be. I added the check for the active casting video as a secondary bit of code. Since we need to scan all the videos anyway, I just look for the active cast in the loop.

> > +        if (!video.paused && unwrappedVideo.mozAllowCasting && !castVideo) {
> 
> Too many patches around, but what is castVideo. Should this be aVideo?

Good catch. That was in the code before I moved the scan loop into updatePageAction. See I did move some code into updatePageAction!

> > +        if (!video.paused && unwrappedVideo.mozAllowCasting && !castVideo) {
> > +          // This <video> is cast-ready. Keep looking so cast-active could be found.
> > +          aVideo = video;
> 
> I wonder if you need to check sources/airplay tags here.

mozAllowCasting already handles that. mozAllowCasting is only true if the video passes all of our tests in CastingApps._getVideo

> > +      // The scan turned up no castable <video>
> > +      if (!aVideo) {
> > +        return;
> 
> Can you pull this video element finder stuff into its own helper function?

I could, but who else is calling it? And didn't you want me to put more into updatePageaction? :)
(In reply to Mark Finkle (:mfinkle) from comment #4)
> > This will fire for every video on the page, not just the one that's being
> > cast...
> 
> Really? Why? handleVideoPlaying should only be called for videos that start
> or finish playing. What am I missing. (hint: if you know a answer, don't
> keep it a secret in the review comment)

If you play two videos on a page (or an audio element starts playing in addition to a video), it will fire. I think you catch the audio problem in handleVideoEvent, but both videos will run through into updatePageAction, and you'll wind up using the second video to determine whether to show the cast icon instead of the video that's currently casting.

> > Can you pull this video element finder stuff into its own helper function?
> 
> I could, but who else is calling it? And didn't you want me to put more into
> updatePageaction? :)

Heh. I don't want more in there. I just wanted to avoid un-necessary misdirection and make it clear in handleEvent function what's happening. Right now handleEvent calls handleVideoCheck whose name confused me. When I looked to see what handleVideoCheck did, it (mostly) just updates the page action (although, I'll admit, it could do more someday). So my first choice for a better name was "updatePageAction" but that's already taken :)

Moving the code into a helper just makes things easier to read.
(In reply to Wesley Johnston (:wesj) from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > > This will fire for every video on the page, not just the one that's being
> > > cast...
> > 
> > Really? Why? handleVideoPlaying should only be called for videos that start
> > or finish playing. What am I missing. (hint: if you know a answer, don't
> > keep it a secret in the review comment)
> 
> If you play two videos on a page (or an audio element starts playing in
> addition to a video), it will fire. I think you catch the audio problem in
> handleVideoEvent, but both videos will run through into updatePageAction,
> and you'll wind up using the second video to determine whether to show the
> cast icon instead of the video that's currently casting.

Ah yes. This code will fire for all "playing" videos on a webpage. You'r initial comment scared me though: "This will fire for every video on the page"

Not every video, just the ones that are playing. I'm not too concerned about that. I have not yet seen many pages that encourage you to play more than one video at a time.

> > > Can you pull this video element finder stuff into its own helper function?
> > 
> > I could, but who else is calling it? And didn't you want me to put more into
> > updatePageaction? :)
> 
> Heh. I don't want more in there. I just wanted to avoid un-necessary
> misdirection and make it clear in handleEvent function what's happening.
> Right now handleEvent calls handleVideoCheck whose name confused me. When I
> looked to see what handleVideoCheck did, it (mostly) just updates the page
> action (although, I'll admit, it could do more someday). So my first choice
> for a better name was "updatePageAction" but that's already taken :)
> 
> Moving the code into a helper just makes things easier to read.

I can move the scanning code into it's own function.
Attached patch casting-pageaction v0.2 (obsolete) — Splinter Review
This patch:
* Removes SCAN_VIDEO
* Renames handleVideoCheck -> _updatePageActionForTab
* Renames handleVideoPlaying -> _updatePageActionForVideo
* Extracts the video scanning code into _findCastableVideo

The patch does not try to cleanly handle the NativeWindow.pageactions.remove call. It was not a simple matter. Not only do we need to remove the old page action if we don't plan on showing one (that was easy), we also need to remove it if the old pageaction was in cast-ready, but the new pageaction is cast-active (and vice versa). That requires keeping some state about the current pageaction (ready or active).

It felt too complicated so I kept the working simple version.
Attachment #8412369 - Attachment is obsolete: true
Attachment #8415004 - Flags: review?(wjohnston)
I forgot to qref
Attachment #8415004 - Attachment is obsolete: true
Attachment #8415004 - Flags: review?(wjohnston)
Attachment #8415908 - Flags: review?(wjohnston)
Comment on attachment 8415908 [details] [diff] [review]
casting-pageaction v0.3

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

Thanks for the cleanup. I like this better. We should maybe file some follow up bugs about defining behaviour if you cast from multiple tabs and casting multiple times from the same tab.

::: mobile/android/chrome/content/CastingApps.js
@@ +262,5 @@
> +      let castableVideo = null;
> +      let videos = aBrowser.contentDocument.querySelectorAll("video");
> +      for (let video of videos) {
> +        let unwrappedVideo = XPCNativeWrapper.unwrap(video);
> +        if (video.paused && unwrappedVideo.mozIsCasting) {

I'm curious, why the paused check here? What if someone starts casting something, and then unpauses it in the page? Can we get by without this?

@@ +267,5 @@
> +          // This <video> is cast-active. Break out of loop.
> +          return video;
> +        }
> +
> +        if (!video.paused && unwrappedVideo.mozAllowCasting) {

This one makes more sense to me. We only show this if a video is playing. We might need to also filter that another castableVideo hasn't been found yet here if you want the first playing video in a page, but I think this is fine...

@@ +300,5 @@
> +      NativeWindow.pageactions.remove(this.pageAction.id);
> +      delete this.pageAction.id;
> +    }
> +
> +    if (!aVideo) {

Reading your comments, I guess you thought this was too much of an edge case. I still think we could just do this video check all the time and if we find a different video is already casting:

1.) Use it for aVideo instead, and just fall through here. Or
2.) if the video that's already casting isn't == aVideo, just bail?

I think that's a pretty simple fix that can keep us from getting into a weird multiple casting state, but I agree it would be odd to get into. I'll leave it to you.

@@ +315,5 @@
> +
> +    // We check for two state here:
> +    // 1. The video is actively being cast
> +    // 2. The video is allowed to be cast and is currently playing
> +    // Both states have the same action: Start casting the video

s/Start casting the video/Show the cast page action/
Attachment #8415908 - Flags: review?(wjohnston) → review+
Comment on attachment 8415908 [details] [diff] [review]
casting-pageaction v0.3

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

::: mobile/android/chrome/content/CastingApps.js
@@ +262,5 @@
> +      let castableVideo = null;
> +      let videos = aBrowser.contentDocument.querySelectorAll("video");
> +      for (let video of videos) {
> +        let unwrappedVideo = XPCNativeWrapper.unwrap(video);
> +        if (video.paused && unwrappedVideo.mozIsCasting) {

I'll land the patch without the paused check and see how it feels. I expect we'll play with this a bit to get it right.

@@ +300,5 @@
> +      NativeWindow.pageactions.remove(this.pageAction.id);
> +      delete this.pageAction.id;
> +    }
> +
> +    if (!aVideo) {

Most of the time we will be scanning for a video, but in those few times that we actually have a video, I think we should use it.

Again, we'll be landing more patches to get this working well enough to ship. Let's see how this feels and fix what we find.
With paused check removed and comment updated:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b0ef55acde3
https://hg.mozilla.org/mozilla-central/rev/0b0ef55acde3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Depends on: 1006973
Comment on attachment 8415908 [details] [diff] [review]
casting-pageaction v0.3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Needed to make casting more discoverable
Testing completed (on m-c, etc.): Works but needs bug 1006973 (already a+)
Risk to taking this patch (and alternatives if risky): low to medium, but works well after bug 1006973
String or IDL/UUID changes made by this patch: none
Attachment #8415908 - Flags: approval-mozilla-aurora?
Attachment #8415908 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in builds:
- 33.0a1 (2014-06-13);
- 32.0a2 (2014-06-13);
Devices: Google Nexus 10 (Android 4.4.2) and Samsung Galaxy R (Android 2.3.4).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.