[Roku] Show "casting" button in the video controls

VERIFIED FIXED in Firefox 33

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: fang, Assigned: mfinkle)

Tracking

Trunk
Firefox 31
Other
Other
Points:
---

Firefox Tracking Flags

(firefox33 verified, firefox34 verified, firefox35 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

The "cast to screen" click feels like an extra click.

Current:
1. Long tapping on video, show "Cast to screen"
2. Click on "Cast to screen", show "Cast to device" & device list
3. Click on device name "Roku streamming device" to start streaming

Expect:
1. Long tapping on video, show "Cast to device" UI
2. Click on device name and start streaming
Sounds like you want long tapping on a video to jump straight to the list of streaming devices.

What about the context menu for a video? I think we still need to keep the regular context menu.

If we want a simpler flow, we should do as Wes suggested and add a "Cast" button to the video controls that appear when you tap a video. One downside to the single tap "controls" is that web pages can override them, so we still need a secondary way of getting to the cast UI.
I am changing this bug to add a "casting/disconnect" button to the video controls XBL binding.
Summary: [Roku] Show "Cast to device UI" after one long tapping click → [Roku] Show "casting" button in the video controls
Created attachment 8401992 [details] [diff] [review]
cast-button-control WIP

This is my first attempt at getting this to work.
* Add a "cast" button to the videocontrols.xml binding.
* Show and hide the button based on the type of the video media (.mp4)
* Fire an event to chrome to start the casting process.

Todo:
* Show and hide the button based on the presence of a casting device on the network.
Comment on attachment 8401992 [details] [diff] [review]
cast-button-control WIP

Thoughts on how to improve this?
Attachment #8401992 - Flags: feedback?(wjohnston)
Created attachment 8402885 [details] [diff] [review]
cast-button-control WIP0.2

Started using CustomEvents after talking to Dolske
Attachment #8401992 - Attachment is obsolete: true
Attachment #8401992 - Flags: feedback?(wjohnston)
Attachment #8402885 - Flags: feedback?(wjohnston)
Created attachment 8403708 [details] [diff] [review]
cast-button-control WIP0.3

This version adds code to let the binding know when Firefox is casting the binding (active=true) and when it's stopped (active=false).

The patch keeps a weak reference to the video element. This should protect us from circular references and allow us to nicely deactivate a video element, even if it's not in the open tab.

The patch adds an "active" state image as well. Both the ready and active state images are from the HDPI Android resources that Google wants applications to use.

TODO:
* Try to move as much of the code out of the base videocontrols binding and put it in the touchcontrols binding.
Attachment #8402885 - Attachment is obsolete: true
Attachment #8402885 - Flags: feedback?(wjohnston)
Attachment #8403708 - Flags: feedback?(wjohnston)
Created attachment 8405721 [details] [diff] [review]
cast-button-control v0.1

This version of the patch moves all of the casting button code and markup into the touchcontrols binding. Nothing of the casting code is in the regular binding.

I did make a few small changes to the touchcontrols binding:

* Added a video data member, instead of just a local in init(). The Utils.video (pulling it from videocontrols) did not always work, since the base videocontrols binding might not be ready and lose the race.

I plan to make a small test for this too, but let the reviews being!
Assignee: nobody → mark.finkle
Attachment #8403708 - Attachment is obsolete: true
Attachment #8403708 - Flags: feedback?(wjohnston)
Attachment #8405721 - Flags: review?(wjohnston)
Attachment #8405721 - Flags: review?(dolske)
Created attachment 8405722 [details]
fennec-video-casting-controls.png

Screenshot of the video controls. The top video is not being cast (white button), but the bottom video is being cast (blue filled button).
Comment on attachment 8405721 [details] [diff] [review]
cast-button-control v0.1

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

A few questions from me. Overall looks good though :) Mostly nits, but a few questions that I'm pretty sure you have answers to.

::: mobile/android/chrome/content/CastingApps.js
@@ +62,5 @@
>          break;
>      }
>    },
>  
> +  handleVideoBindingAttached: function handleVideoBindingAttached(aTab, aEvent) {

The "everything should just do one thing" in me guy thinks this should be in a separate class.

@@ +81,5 @@
> +
> +    // Let the binding know casting is allowed
> +    let event = video.ownerDocument.createEvent("CustomEvent");
> +    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ allow: true }));
> +    video.dispatchEvent(event);

This won't dynamically adjust. Nor do we handle cases where we're already casting something and you open it again very well.

@@ +297,5 @@
> +    let video = this.session.videoRef.get();
> +    if (video) {
> +      let event = video.ownerDocument.createEvent("CustomEvent");
> +      event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ active: true }));
> +      video.dispatchEvent(event);

You might as well wrap this up in a helper function.

_sendEventToVideo(video, {active: true});

::: toolkit/content/widgets/videocontrols.xml
@@ +1655,5 @@
> +              let unwrappedVideo = XPCNativeWrapper.unwrap(this.video);
> +              let castingData = JSON.parse(eventDetail);
> +              if ("allow" in castingData) {
> +                if (castingData.allow)
> +                  unwrappedVideo.mozAllowCasting = true;

Why is this stored on the video and not in the binding.

@@ +1662,5 @@
> +              }
> +
> +              if ("active" in castingData) {
> +                if (castingData.active)
> +                  unwrappedVideo.mozIsCasting = true;

And this?

@@ +1670,5 @@
> +              this.setCastButtonState();
> +            },
> +
> +            startCasting : function () {
> +              this.videocontrols.dispatchEvent(new CustomEvent("VideoBindingCast"));

It confuses me a bit that this doesn't toggle the state, but after talking it through with you, it sounds worth testing this way for now, and adjusting once we have some users.

@@ +1681,5 @@
> +                return;
> +              }
> +
> +              if (unwrappedVideo.mozIsCasting) {
> +                this.castingButton.setAttribute("active", "true");

'active' is a strange attribute name, since its also a pseudoclass. I have nothing better though :)

@@ +1686,5 @@
> +              } else {
> +                this.castingButton.removeAttribute("active");
> +              }
> +
> +              var value = this.castingButton.getAttribute("castinglabel");

I'm sure there's a good reason for this, but why not just set it directly?
Attachment #8405721 - Flags: review?(wjohnston) → review+
Comment on attachment 8405721 [details] [diff] [review]
cast-button-control v0.1

Looks ok to me. Wesley's review if fine for the touch/mobile bits.

Nit: Might want to ponder about something more descriptive for "cast to device". Just because "device" seems like such an ambiguous term (and we usually use it to refer to mobile devices).
Attachment #8405721 - Flags: review?(dolske) → review+
(In reply to Wesley Johnston (:wesj) from comment #9)

> ::: mobile/android/chrome/content/CastingApps.js

> > +  handleVideoBindingAttached: function handleVideoBindingAttached(aTab, aEvent) {
> 
> The "everything should just do one thing" in me guy thinks this should be in
> a separate class.

I do too, but CastingApps is the best place for it IMO. I don't want to scatter video casting code around too much.

> > +    let event = video.ownerDocument.createEvent("CustomEvent");
> > +    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ allow: true }));
> > +    video.dispatchEvent(event);
> 
> This won't dynamically adjust. Nor do we handle cases where we're already
> casting something and you open it again very well.

Right. That is more complicated and I wanted to experiement with it in a different bug. I don't think it blocks getting the first pass of this code landed.

> > +      let event = video.ownerDocument.createEvent("CustomEvent");
> > +      event.initCustomEvent("media-videoCasting", false, true, JSON.stringify({ active: true }));
> > +      video.dispatchEvent(event);
> 
> You might as well wrap this up in a helper function.
> 
> _sendEventToVideo(video, {active: true});

Done

> ::: toolkit/content/widgets/videocontrols.xml

> > +                if (castingData.allow)
> > +                  unwrappedVideo.mozAllowCasting = true;
> 
> Why is this stored on the video and not in the binding.
> 

> > +              if ("active" in castingData) {
> > +                if (castingData.active)
> > +                  unwrappedVideo.mozIsCasting = true;
> 
> And this?

The existing code using this pattern to allow the chrome JS to check for state without exposing it to content.

> > +              var value = this.castingButton.getAttribute("castinglabel");
> 
> I'm sure there's a good reason for this, but why not just set it directly?

I was following the pattern in the file, but you're right. We can just set it directly.


(In reply to Justin Dolske [:Dolske] from comment #10)

> Nit: Might want to ponder about something more descriptive for "cast to
> device". Just because "device" seems like such an ambiguous term (and we
> usually use it to refer to mobile devices).

You're right. "Device" is not what I wanted anyway. "Screen" is better suited, so now the string is "Cast to Screen".
https://hg.mozilla.org/integration/mozilla-inbound/rev/45a45f4a228f

leave open. i promised a test.
Keywords: leave-open
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31

Comment 15

4 years ago
Verified as fixed in builds:
- 35.0a1 (2014-09-30);
- 34.0a2 (2014-09-30);
- 33 beta 7;
Device: Asus Transformer Tab (Android 4.2.1).
Status: RESOLVED → VERIFIED
status-firefox33: --- → verified
status-firefox34: --- → verified
status-firefox35: --- → verified
You need to log in before you can comment on or make changes to this bug.