Closed Bug 1054959 Opened 10 years ago Closed 10 years ago

Add 'Send Video To Device' to the context menu for sending videos from desktop to a second screen

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 36
Iteration:
36.1
Tracking Status
firefox35 + disabled
firefox36 --- verified

People

(Reporter: krudnitski, Assigned: blassey)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Keywords: late-l10n)

Attachments

(1 file, 10 obsolete files)

20.12 KB, patch
Details | Diff | Splinter Review
Firefox for Android has been working to implement sending videos and sending tabs (aka video casting and tab mirroring) from fennec to Roku and Chromecast (as a start). See bug 921924.

The intention is that the design work already done can be repurposed for use on desktop, as well as much of the code.

We would like this extended to Firefox desktop in order to complete the story from a Firefox ecosystem perspective. Chromecast has set the bar in supporting both mobile & desktop in user expectations. Firefox for Android is targeting Firefox 33 for implementation of sending videos to the Roku and Chromecast, and tabs will either squeak in or be supported in Firefox 34.
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Flags: qe-verify?
here's an addon that will send videos to Roku http://dump.lassey.us/fling.xpi
Depends on: 1064834
Attached patch WIP (obsolete) — Splinter Review
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8491649 - Attachment is obsolete: true
Attachment #8491729 - Flags: review?(gavin.sharp)
Comment on attachment 8491729 [details] [diff] [review]
casting_apps_desktop.patch

There's a bunch of dead code in this patch. What's the story about that?

Does this patch work? I don't know how I would test it.

The context menu stuff should live with the other context menu things rather than being dynamically added here.
Attachment #8491729 - Flags: review?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #4)
> Comment on attachment 8491729 [details] [diff] [review]
> casting_apps_desktop.patch
> 
> There's a bunch of dead code in this patch. What's the story about that?
The dead code I see is places where Fennec has hooks for playback controls. I figured it was better to leave those there than to have Firefox frontend devs reinvent those wheels

> Does this patch work? I don't know how I would test it.
Yes. You can test it with the Firefox channel on roku

> The context menu stuff should live with the other context menu things rather
> than being dynamically added here.
Got a pointer for where to look?
Flags: needinfo?(gavin.sharp)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> The dead code I see is places where Fennec has hooks for playback controls.
> I figured it was better to leave those there than to have Firefox frontend
> devs reinvent those wheels

Not sure what you mean. I meant things like "filterCast" and "pageAction" and getVideo/_getVideo in that patch not being called by anything. Why add them if they're not being used? Is the plan to have them be used later?

> Yes. You can test it with the Firefox channel on roku

Yeah, I don't have one of those :)

> Got a pointer for where to look?

browser-context.inc is where you define the menu item, nsContextMenu.js is where you add the logic for displaying it/making it work.

Some other observations:
- I think you mentioned this not being e10s compatible, we can't land it like that
- the decideToShowMyMenuItem function should not do nodeName checks, probably wants "instanceof HTMLVideoElement" (though nsContextMenu has helpers for this)
- There seems to be a dependency on RokuApp.jsm, but that's not imported in this patch?
Flags: needinfo?(gavin.sharp)
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
Attachment #8491729 - Attachment is obsolete: true
Attachment #8497857 - Flags: review?(gavin.sharp)
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #6)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> > The dead code I see is places where Fennec has hooks for playback controls.
> > I figured it was better to leave those there than to have Firefox frontend
> > devs reinvent those wheels
> 
> Not sure what you mean. I meant things like "filterCast" and "pageAction"
> and getVideo/_getVideo in that patch not being called by anything. Why add
> them if they're not being used? Is the plan to have them be used later?
getVideo/_getVideo are used. The other two were cruft.
> 
> > Yes. You can test it with the Firefox channel on roku
> 
> Yeah, I don't have one of those :)
> 
> > Got a pointer for where to look?
> 
> browser-context.inc is where you define the menu item, nsContextMenu.js is
> where you add the logic for displaying it/making it work.
> 
> Some other observations:
> - I think you mentioned this not being e10s compatible, we can't land it
> like that
works fine with e10s
> - the decideToShowMyMenuItem function should not do nodeName checks,
> probably wants "instanceof HTMLVideoElement" (though nsContextMenu has
> helpers for this)
that stuff is gone
> - There seems to be a dependency on RokuApp.jsm, but that's not imported in
> this patch?
that was added in bug 1064834, which blocks this one
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> > Not sure what you mean. I meant things like "filterCast" and "pageAction"
> > and getVideo/_getVideo in that patch not being called by anything. Why add
> > them if they're not being used? Is the plan to have them be used later?
> getVideo/_getVideo are used. The other two were cruft.

I didn't see any calls to getVideo attachment 8491729 [details] [diff] [review] except for the one in filterCast.

> > - There seems to be a dependency on RokuApp.jsm, but that's not imported in
> > this patch?
> that was added in bug 1064834, which blocks this one

Right, but there was no import() call for it in attachment 8491729 [details] [diff] [review].
Comment on attachment 8497857 [details] [diff] [review]
casting_apps_desktop.patch

We don't use the "a" prefix for parameters in new code, and this would be a lot cleaner with arrow functions rather than normal callbacks and bind()s.

The fact that this code necessarily entrains the loading of CastingApps.js/SimpleServiceDiscovery.jsm/RokuApp.jsm for all users, even though most won't have Roku devices, is rather unfortunate from a memory use point of view. Can we find a cheaper way to detect whether a device exists? Perhaps splitting the "device detection" part of SimpleServiceDiscovery into its own JSM, and lazy-loading everything else only if it finds something?

>diff --git a/browser/base/content/CastingApps.js b/browser/base/content/CastingApps.js

>+var CastingApps = {

>+  init: function ca_init() {
>+    Cu.import("resource://gre/modules/SimpleServiceDiscovery.jsm");

Should just import this at the top.

>+    // Search for devices continuously every 120 seconds
>+    SimpleServiceDiscovery.search(120 * 1000);

Isn't this bad for battery life? (causes a lot of otherwise unnecessary wakeups).

>+  initWindow: function initWindow(aDOMWindow) {
>+    /*var contentAreaContextMenu = aDOMWindow.document.getElementById('contentAreaContextMenu');

>+  uninitWindow: function ca_uninit(aDOMWindow) {
>+    /*var contentAreaContextMenu = aDOMWindow.document.getElementById('contentAreaContextMenu');

Looks like we don't need these anymore. I guess you will need to find some other way to ensure CastingApps.init() is called near startup.

>+  _sendEventToVideo: function _sendEventToVideo(aElement, aData) {
>+    let event = aElement.ownerDocument.createEvent("CustomEvent");
>+    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify(aData));
>+    aElement.dispatchEvent(event);

So does this rely on CPOWs then? This runs in the parent process and aElement is a content element.

Why do we even send the events to videos?

>+  getVideo: function(aElement, aX, aY) {

>+    // 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.

I don't think this code will ever be hit given your nsContextMenu changes (this will only be invoked when aElement is a video element), so we should be able to rip it all out.

>+  _getVideo: function(aElement, aTypes, aExtensions) {

>+    // First, look to see if the <video> has a src attribute
>+    let sourceURL = aElement.src;
>+
>+    // If empty, try the currentSrc
>+    if (!sourceURL) {
>+      sourceURL = aElement.currentSrc;
>+    }

Not an expert on the mediaelement API, but this seems weird. What's the rationale for trying src first but falling back to currentSrc?

>+    if (sourceURL) {
>+      // Use the file extension to guess the mime type
>+      let sourceURI = this.makeURI(sourceURL, null, this.makeURI(aElement.baseURI));

I think you can use aElement.baseURIObject here instead of makeURI(aElement.baseURI). But I think if you just always use currentSrc, that should be an absolute URI fully resolved against the right base and you can just get rid of all of this.

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

This should be reflected by currentSrc as I understand it, so it's unnecessary.

>+  _findCastableVideo: function _findCastableVideo(aBrowser) {

This is unused.

>+  prompt: function(aCallback, aFilterFunc, aWindow) {

>+    var selected = {}
>+    var promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"]
>+      .getService(Ci.nsIPromptService);
>+    if (!promptService.select(aWindow, "select a device", "select the device to cast to", items.length, items, selected))
>+      return;

Need proper l10n here (you can put strings in browser.properties).

>+  openExternal: function(aElement) {

>+    function filterFunc(service) {
>+      return this.allowableExtension(video.sourceURI, service.extensions) || this.allowableMimeType(video.type, service.types);
>+    }

Why not just put this in prompt() directly? Given the new function definitions I'm proposing below you could probably inline this even further.

>+      app.stop(function() {
>+        app.start(function(aStarted) {
>+          if (!aStarted) {
>+            dump("CastingApps: Unable to start app");

Shouldn't have these dump()s in here. I didn't review this stuff closely since I'm not familiar with this API, but I assume it works.

>+          }.bind(this), this);
>+        }.bind(this));
>+      }.bind(this));
>+    }.bind(this), filterFunc.bind(this), window);

Use fat arrow functions and this stuff won't be necessary. Unfortunate that these APIs use callbacks rather than promises.

>+  allowableExtension: function(aURI, aExtensions) {
>+    if (aURI && aURI instanceof Ci.nsIURL) {
>+      for (let x in aExtensions) {
>+        if (aURI.fileExtension == aExtensions[x]) return true;
>+      }
>+    }

Null check is redundant, and you can use Array.contains:

(uri, extensions) => { (uri instanceof Ci.nsIURL) && extensions.contains(uri.fileExtension); }

>+  allowableMimeType: function(aType, aTypes) {
>+    for (let x in aTypes) {
>+      if (aType == aTypes[x]) return true;
>+    }
>+    return false;

Same here: (type, types) => types.contains(type)

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+XPCOMUtils.defineLazyGetter(this, "CastingApps", function() {
>+  let tmp = {};
>+  Services.scriptloader.loadSubScript("chrome://browser/content/CastingApps.js", tmp);

This should just be a JSM, looks like?

>diff --git a/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm b/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm

>   _usingLAN: function() {
>     let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
>-    return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
>+    return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI ||
>+            network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET ||
>+            network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_UNKNOWN);

Intentional change?
Attachment #8497857 - Flags: review?(gavin.sharp) → review-
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #10)

> The fact that this code necessarily entrains the loading of
> CastingApps.js/SimpleServiceDiscovery.jsm/RokuApp.jsm for all users, even
> though most won't have Roku devices, is rather unfortunate from a memory use
> point of view. Can we find a cheaper way to detect whether a device exists?
> Perhaps splitting the "device detection" part of SimpleServiceDiscovery into
> its own JSM, and lazy-loading everything else only if it finds something?

RokuApp.jsm will only be imported if we find a Roku device _and_ the user wants to cast. The rokuDevice helper object defers loading the RokuApp.jsm file.

If we can split SimpleServiceDiscovery into smaller parts that would be great. I don't think there is much we can do though. We need to ping the network for second screen devices. Those pings need target info. Each type of device has a different set of target info, so we need to register devices and hold some metadata.

We could defer loading SimpleServiceDiscovery in CastingApps if a global "casting" preference is disabled.
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #10)

> >diff --git a/browser/base/content/CastingApps.js b/browser/base/content/CastingApps.js
> 
> >+var CastingApps = {
> 
> >+  init: function ca_init() {
> >+    Cu.import("resource://gre/modules/SimpleServiceDiscovery.jsm");
> 
> Should just import this at the top.

Not if you want to try to defer loading it.

> >+    // Search for devices continuously every 120 seconds
> >+    SimpleServiceDiscovery.search(120 * 1000);
> 
> Isn't this bad for battery life? (causes a lot of otherwise unnecessary
> wakeups).

We use this on mobile, but a phone and/or tablet move a round a lot more than a laptop or desktop, so maybe a longer timeout is OK. The ping only happens if we're on Wifi. Maybe we could put a check in SimpleServiceDiscovery to check on the battery too, and bail if power level is bleow a threshold.

> >+  _sendEventToVideo: function _sendEventToVideo(aElement, aData) {
> >+    let event = aElement.ownerDocument.createEvent("CustomEvent");
> >+    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify(aData));
> >+    aElement.dispatchEvent(event);
> 
> So does this rely on CPOWs then? This runs in the parent process and
> aElement is a content element.

I can't answer that...

> Why do we even send the events to videos?

So the <video> binding can show a casting icon in its control set

> >+  _getVideo: function(aElement, aTypes, aExtensions) {
> 
> >+    // First, look to see if the <video> has a src attribute
> >+    let sourceURL = aElement.src;
> >+
> >+    // If empty, try the currentSrc
> >+    if (!sourceURL) {
> >+      sourceURL = aElement.currentSrc;
> >+    }
> 
> Not an expert on the mediaelement API, but this seems weird. What's the
> rationale for trying src first but falling back to currentSrc?

src might not be set, or could be different than the actual playing src once the video is playing.

> >+    // Next, look to see if there is a <source> child element that meets
> >+    // our needs
> 
> This should be reflected by currentSrc as I understand it, so it's
> unnecessary.

Only if the video is actually playing. If we happen upon a video on the page that is not playing, currentSrc is not useful, but the child <source> elemtns are.

> Use fat arrow functions and this stuff won't be necessary. Unfortunate that
> these APIs use callbacks rather than promises.

Booo

> >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> 
> >+XPCOMUtils.defineLazyGetter(this, "CastingApps", function() {
> >+  let tmp = {};
> >+  Services.scriptloader.loadSubScript("chrome://browser/content/CastingApps.js", tmp);
> 
> This should just be a JSM, looks like?

In fennec, CastinApps is a JS loaded into the browser.js scope so we have the window and document. At least that was the original reason. Maybe that could change.
(In reply to Mark Finkle (:mfinkle) from comment #12)
> > Should just import this at the top.
> 
> Not if you want to try to defer loading it.

CastingApps' init is called right after import, and it uses SimpleServiceDiscovery, so there's no benefit to lazy loading here.

> > Not an expert on the mediaelement API, but this seems weird. What's the
> > rationale for trying src first but falling back to currentSrc?
> 
> src might not be set, or could be different than the actual playing src once
> the video is playing.

But why isn't currentSrc always what you want?

> Only if the video is actually playing. If we happen upon a video on the page
> that is not playing, currentSrc is not useful, but the child <source>
> elemtns are.

That seems pretty unfortunate. Seems like we should get some better API here rather than rolling our own media selection logic (which will get out of sync with the underlying mediaelement code).

> > This should just be a JSM, looks like?
> 
> In fennec, CastinApps is a JS loaded into the browser.js scope so we have
> the window and document. At least that was the original reason. Maybe that
> could change.

You can pass the window into the JSM as needed (though perhaps that suggests a refactoring where the window-dependent/UI code is factored out).
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #10)
> Comment on attachment 8497857 [details] [diff] [review]
> casting_apps_desktop.patch
> 
> We don't use the "a" prefix for parameters in new code, and this would be a
> lot cleaner with arrow functions rather than normal callbacks and bind()s.
> 
> The fact that this code necessarily entrains the loading of
> CastingApps.js/SimpleServiceDiscovery.jsm/RokuApp.jsm for all users, even
> though most won't have Roku devices, is rather unfortunate from a memory use
> point of view. Can we find a cheaper way to detect whether a device exists?
> Perhaps splitting the "device detection" part of SimpleServiceDiscovery into
> its own JSM, and lazy-loading everything else only if it finds something?

RokuApp.jsm isn't loaded unless SimpleServiceDiscovery finds a roku.

 
> >diff --git a/browser/base/content/CastingApps.js b/browser/base/content/CastingApps.js
> 
> >+var CastingApps = {
> 
> >+  init: function ca_init() {
> >+    Cu.import("resource://gre/modules/SimpleServiceDiscovery.jsm");
> 
> Should just import this at the top.
> 
> >+    // Search for devices continuously every 120 seconds
> >+    SimpleServiceDiscovery.search(120 * 1000);
> 
> Isn't this bad for battery life? (causes a lot of otherwise unnecessary
> wakeups).
> 
> >+  initWindow: function initWindow(aDOMWindow) {
> >+    /*var contentAreaContextMenu = aDOMWindow.document.getElementById('contentAreaContextMenu');
> 
> >+  uninitWindow: function ca_uninit(aDOMWindow) {
> >+    /*var contentAreaContextMenu = aDOMWindow.document.getElementById('contentAreaContextMenu');
> 
> Looks like we don't need these anymore. I guess you will need to find some
> other way to ensure CastingApps.init() is called near startup.
yup, meant to delete that

> 
> >diff --git a/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm b/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm
> 
> >   _usingLAN: function() {
> >     let network = Cc["@mozilla.org/network/network-link-service;1"].getService(Ci.nsINetworkLinkService);
> >-    return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI || network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET);
> >+    return (network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_WIFI ||
> >+            network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_ETHERNET ||
> >+            network.linkType == Ci.nsINetworkLinkService.LINK_TYPE_UNKNOWN);
> 
> Intentional change?

Yup, added UNKNOWN
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #13)
> (In reply to Mark Finkle (:mfinkle) from comment #12)

 
> > > Not an expert on the mediaelement API, but this seems weird. What's the
> > > rationale for trying src first but falling back to currentSrc?
> > 
> > src might not be set, or could be different than the actual playing src once
> > the video is playing.
> 
> But why isn't currentSrc always what you want?
This is needed to be able to send an mp4 to the roku when firefox is playing a webm video (roku doesn't support webm).

 
> > Only if the video is actually playing. If we happen upon a video on the page
> > that is not playing, currentSrc is not useful, but the child <source>
> > elemtns are.
> 
> That seems pretty unfortunate. Seems like we should get some better API here
> rather than rolling our own media selection logic (which will get out of
> sync with the underlying mediaelement code).

What would get out of sync?
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
Attachment #8497857 - Attachment is obsolete: true
Attachment #8498627 - Flags: review?(gavin.sharp)
Attachment #8498627 - Flags: review?(gavin.sharp) → review?(dtownsend+bugmail)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> What would get out of sync?

The logic for which sources to prefer, which are supported, etc. Ideally there would be an API like videoElement.getSourceURL("h264") or somesuch. Not going to get this here, so just making it work is fine.
Comment on attachment 8498627 [details] [diff] [review]
casting_apps_desktop.patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+XPCOMUtils.defineLazyGetter(this, "CastingApps", function() {
>+  let tmp = {};
>+  Services.scriptloader.loadSubScript("resource:///modules/CastingApps.jsm", tmp);
>+  tmp["CastingApps"].init();
>+  return tmp["CastingApps"];
>+});

Use XPCOMUtils.defineLazyModuleGetter, and then just put the call to init() somewhere in the startup path.

I still think we need to address the memory use concern somehow. One way to avoid needing to load CastingApps in the common case would be to just inline CastingApps.init() to this browser startup code (moving over the RokuApp definition as well). That code should probably live in nsBrowserGlue rather than browser.js.

>+    CastingApps.initWindow(window);

>+    CastingApps.uninitWindow(window);

remove these

>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY castVideoCmd.label           "Cast Videoâ¦">
> <!ENTITY emailAudioCmd.label          "Email Audioâ¦">
> <!ENTITY emailAudioCmd.accesskey      "a">

No accesskey?

>diff --git a/browser/locales/en-US/chrome/browser/browser.properties b/browser/locales/en-US/chrome/browser/browser.properties

>+casting.selectDevice.title = Select a device
>+casting.selectDevice.msg = select the device to cast to
>\ No newline at end of file

Add a newline. The second "Select" should be capitalized I assume.

This needs UI review, can you file a UX bug for that?

>diff --git a/browser/modules/CastingApps.jsm b/browser/modules/CastingApps.jsm

>+var CastingApps = {

>+  initWindow: function initWindow(aDOMWindow) {
>+  },
>+
>+  uninitWindow: function ca_uninit(aDOMWindow) {
>+  },

Remove these?

>+  _sendEventToVideo: function _sendEventToVideo(aElement, aData) {
>+    let event = aElement.ownerDocument.createEvent("CustomEvent");
>+    event.initCustomEvent("media-videoCasting", false, true, JSON.stringify(aData));
>+    aElement.dispatchEvent(event);

Still no answer about the CPOW use here. I guess if it works it's fine, but still curious how it works.


>+      app.stop(() => {
>+        app.start((aStarted) => {
>+          if (!aStarted) {
>+            dump("CastingApps: Unable to start app");

Still need to remove these dump()s (or change to Cu.reportError())
Attachment #8498627 - Flags: review?(dtownsend+bugmail) → review-
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #17)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #15)
> > What would get out of sync?
> 
> The logic for which sources to prefer, which are supported, etc. Ideally
> there would be an API like videoElement.getSourceURL("h264") or somesuch.
> Not going to get this here, so just making it work is fine.

I like it, filed bug 1079407
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
screenshots in comment 20 for ui review
Attachment #8498627 - Attachment is obsolete: true
Attachment #8501343 - Flags: ui-review?(madhava)
Attachment #8501343 - Flags: review?(gavin.sharp)
Depends on: 1079541
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
Attachment #8501343 - Attachment is obsolete: true
Attachment #8501343 - Flags: ui-review?(madhava)
Attachment #8501343 - Flags: review?(gavin.sharp)
Attachment #8501373 - Flags: ui-review?(madhava)
Attachment #8501373 - Flags: review?(gavin.sharp)
For what to put in the video menu, here's a conversation I just had with Brad in IRC:

16:53 madhava: not to scope creep this -- but sevaan points out that this menu
16:53 madhava: https://www.dropbox.com/s/e8xn2e51wjqwk0f/Screenshot%202014-10-07%2017.22.54.png?dl=0
16:53 madhava: could really benefit from some of the things we do for context menus now
16:54 madhava: http://cl.ly/image/1I3A1o0S1C3z
16:54 madhava: an icon row for
16:54 madhava: Play/Pause | Mute | Full Screen | Fling could be awesome
Where by "Fling" I mean cast or stream or Send to or whatever we mean to call it here. The icon could be a TV screen or something along those lines.

Also, for the second screenshot (the list of devices) -- we should at least make it only tab-modal, not browser-modal. Depending on when we know the list of devices, maybe this wouldn't be a fully second step but could be a sub menu? Or, if there's only one device, we just say where it's being sent, transiently.
Attachment #8501373 - Flags: ui-review?(madhava)
(In reply to Madhava Enros [:madhava] from comment #23)

> 16:54 madhava: an icon row for
> 16:54 madhava: Play/Pause | Mute | Full Screen | Fling could be awesome

I think that's a fantastic idea, but also totally scope-creep. New bug?
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
Attachment #8501373 - Attachment is obsolete: true
Attachment #8501373 - Flags: review?(gavin.sharp)
Attachment #8502194 - Flags: review?(gavin.sharp)
this now uses a submenu instead of a select dialog. It now looks like this https://www.dropbox.com/s/3ftmzbjzzho2mkg/Screenshot%202014-10-08%2021.31.05.png?dl=0
Blocks: 1080809
Bug 1080809 has been created for adding an icon row to media player context menus.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #27)
> this now uses a submenu instead of a select dialog. It now looks like this
> https://www.dropbox.com/s/3ftmzbjzzho2mkg/Screenshot%202014-10-08%2021.31.05.
> png?dl=0

Thanks - this is a _much_ better interaction than the followup modal.
OK - so we've got a followup for the better UI, and I've put the design bug (Bug 1080809) in for the next desktop sprint.

It sounds like the last thing here is the text for the menu item, so that we have something for l10n. I'm not a huge fan of "Send to..." here if it's just going to be about sending a video to a Roku/etc. It's vague, but more importantly it uses up a very important concept ("send to") that it's possible we'd want to reserve for soemthing like tab mirroring or pushing a tab to your phone, other computer, etc. Let alone the conceptual clash with email.

That said - this is what it says on Android right now and we don't want to be inconsistent. ALSO - if it's used here and no more widely, the context _is_ relatively understandable in that it's an action directly on a video, and the resulting options will explain to where.

So, on the whole, let's stick with the "Send to..." language. And we need to figure out how all of these various sending things around features work and how we group them (and under what label(s)) in the browser.
Attached patch casting_apps_desktop.patch (obsolete) — Splinter Review
No with the finalized string from Madhava, "Send Video To Device…"
Attachment #8502194 - Attachment is obsolete: true
Attachment #8502194 - Flags: review?(gavin.sharp)
Attachment #8503412 - Flags: ui-review+
Attachment #8503412 - Flags: review?(gavin.sharp)
Attachment #8504810 - Flags: review?(gavin.sharp)
Comment on attachment 8504810 [details] [diff] [review]
casting_apps_desktop_string.patch

I think there shouldn't be an ellipsis for a context menu item that's just a menupopup parent.
Attachment #8504810 - Flags: review?(gavin.sharp) → review+
<madhava_> no elipsis
This is late-l10n at this point, so you should coordinate with l10n folks for the landing.
Keywords: late-l10n
Flags: qe-verify? → qe-verify+
OS: Windows 8.1 → All
Hardware: x86_64 → All
Attached patch modified patch (obsolete) — Splinter Review
I made a few modifications to your patch to address style comments, and just one substantial change: I adjusted the logic in nsContextMenu to address an issue where we used different code to determine whether to show the menu item and to populate it - in theory this could have resulted in the menu item showing but the popup being empty, if you had services installed that didn't support the video you were clicking on. This is the first nsContextMenu.js hunk.

I think another issue with this code as-is is that the way that getVideo and getServicesForVideo interact, we're not actually optimizing for maximizing the number of compatible services we offer. If you had multiple services with different types supported, this code could result in you only seeing one or another for a given video, even if that video had source types for both. This can be fixed in a followup, though, since that's a relatively unlikely scenario in the near term.

I haven't tested this patch, since I can't easily do that, and so it's possibly I made a dumb typo somewhere. Can you test this and adjust as needed?

Speaking of tests, we really should have one for this UI (and the backend code), but I assume there's no test infra built for this stuff, and that it would require mocking some services/devices/etc. Get a followup filed for that at least?
Attachment #8503412 - Attachment is obsolete: true
Attachment #8503412 - Flags: review?(gavin.sharp)
Attachment #8505178 - Flags: review+
Attachment #8505178 - Flags: feedback?(blassey.bugs)
Comment on attachment 8505178 [details] [diff] [review]
modified patch

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

there is some sort of typo here, trying to figure out what it is. I'm getting: 

XML Parsing Error: undefined entity
Location: chrome://browser/content/browser.xul
Line Number 763, Column 7:      <menuitem id="context-sharelink"
------^
Did you perhaps not have the strings part of the patch (browser.dtd) applied? Or maybe didn't rebuild properly after juggling the strings-only/complete patches? I think that would cause that.
Summary: Add 'send videos' and 'send tabs' from desktop to a second screen → Add 'Send Video To Device' to the context menu for sending videos from desktop to a second screen
Comment on attachment 8505178 [details] [diff] [review]
modified patch

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

In what turns out to be a colossal waste of time, I've been testing a different object directory than the one I've been building. This looks fine and I'll push it without the elipsis.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +483,5 @@
>  <!ENTITY emailImageCmd.label          "Email Image…">
>  <!ENTITY emailImageCmd.accesskey      "g">
>  <!ENTITY emailVideoCmd.label          "Email Video…">
>  <!ENTITY emailVideoCmd.accesskey      "a">
> +<!ENTITY castVideoCmd.label           "Send Video To Device…">

I'm removing the elipsis in the patch I'm pushing.
Attachment #8505178 - Flags: feedback?(blassey.bugs) → feedback+
Comment on attachment 8505178 [details] [diff] [review]
modified patch

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

In what turns out to be a colossal waste of time, I've been testing a different object directory than the one I've been building. This looks fine and I'll push it without the elipsis.

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +483,5 @@
>  <!ENTITY emailImageCmd.label          "Email Image…">
>  <!ENTITY emailImageCmd.accesskey      "g">
>  <!ENTITY emailVideoCmd.label          "Email Video…">
>  <!ENTITY emailVideoCmd.accesskey      "a">
> +<!ENTITY castVideoCmd.label           "Send Video To Device…">

I'm removing the elipsis in the patch I'm pushing.
Attachment #8505178 - Flags: approval-mozilla-aurora?
for the uplift request (strangely there was no comment box when I requested), there is a string added with this patch. We're looking to uplift so it is available to users when Firefox for Android goes out with this feature in 33.
(In reply to Wes Kocher (:KWierso) from comment #42)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/095f351a7386
> for mochitest-1 bustage:
> 
> https://treeherder.mozilla.org/ui/logviewer.html#?job_id=914137&repo=fx-team

menu context-castvideo has same accesskey as context-media-hidecontrols 

Madhava, Darrin, what accesskey should we have here?
Flags: needinfo?(madhava)
Flags: needinfo?(dhenein)
Flags: needinfo?(blassey.bugs)
pushed to try using 'd' for the access key https://tbpl.mozilla.org/?tree=Try&rev=5ae94aef5fa9
Without knowing which access keys are available, I'd say your suggestion of using 'd' (presumably for Device) sounds reasonable. Madhava, any objections?
Flags: needinfo?(dhenein)
test_contextmenu.html is still failing. Backed out :(
https://hg.mozilla.org/integration/fx-team/rev/2a0d75a59098
Blocks: 1084035
I changed the accesskey to "e", which was free, and pushed with a revised version of test_contextmenu.html:

https://hg.mozilla.org/integration/fx-team/rev/eb1181e4493c
Attached patch final patchSplinter Review
Attachment #8504810 - Attachment is obsolete: true
Attachment #8505178 - Attachment is obsolete: true
Attachment #8505178 - Flags: approval-mozilla-aurora?
Flags: needinfo?(madhava)
Attachment #8507236 - Flags: approval-mozilla-aurora+
I pushed this to Aurora with the 2-string change:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ed68a14922fe

Pike/flod: please let me know if you think this needs additional communication/coordination for the late-l10n change.
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
Target Milestone: --- → Firefox 36
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #41)
> for the uplift request (strangely there was no comment box when I
> requested), there is a string added with this patch. We're looking to uplift
> so it is available to users when Firefox for Android goes out with this
> feature in 33.

Firefox 33? Maybe 35 (current aurora)?

Sent email to dev-l10n, but they'll see the new strings in their dashboard.
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
https://hg.mozilla.org/mozilla-central/rev/eb1181e4493c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Iteration: --- → 36.1
Depends on: 1086278
Depends on: 1083952
Depends on: 1072931
Blocks: 1088758
Backed out from Aurora (with the dtd change left intact) until the leaks in bug 1083952 can be sorted out.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed94bc5a9ace
Depends on: 1102482
Depends on: 1111967
I tried to verify this bug, but the 'Send Video to Device' option from the contextmenu is grayed out even if I have a Roku device connected to the same wifi network as the desktop pc.
Flags: needinfo?(blassey.bugs)
Two things could be going wrong for you. First, it only works for mp4 videos (not webM), here is a good test page http://people.mozilla.org/~mfinkle/casting/test.html. Second, some routers are configured in such a way that blocks Firefox from discovering the Roku.
Flags: needinfo?(blassey.bugs)
(In reply to Flaviu Cos, QA [:flaviu] from comment #56)
> I tried to verify this bug, but the 'Send Video to Device' option from the
> contextmenu is grayed out even if I have a Roku device connected to the same
> wifi network as the desktop pc.

My guess is that desktop is not seeing the roku device even though they are on the same wi-fi. For example in MTV, I have to used the ateam network to do testing.
Also tested using this site:  http://people.mozilla.org/~mfinkle/casting/test.html
The 'Send Video to Device' option from the contextmenu is grayed out for all three videos from the website (mp4 and webm).
I can successfully cast to Roku using Firefox installed on an Android device connected to the same wifi network.
Also on Android there is about:devices where I can view all the discovered devices.
Flags: needinfo?(blassey.bugs)
rbarker says it works for him. I don't have a Roku with me in MV to test
Flags: needinfo?(blassey.bugs)
Also works for me.
Actually, I am able to replicate what Flaviu sees.  If I freshly power up the Roku, I can do a few MP4 sends.  Eventually I'm seeing the send option grayed out.  But it's unclear which side is causing it.
(In reply to Christopher Arnold from comment #62)
> Actually, I am able to replicate what Flaviu sees.  If I freshly power up
> the Roku, I can do a few MP4 sends.  Eventually I'm seeing the send option
> grayed out.  But it's unclear which side is causing it.

The option is grayed out when the roku device has not been discovered yet. Since discovery uses an unreliable protocol, it is possible for the roku to miss the discovery requests and also for firefox to miss the discovery responses. Right now the discovery message is sent three time (once a second) every two minutes. If the initial discovery fails, it will take two minutes before firefox try again. It is usually faster to just restart firefox so it will retry discovery immediately.
So it sounds like this works but you don't always get the context menu send option. Does this need to be filed separately and close this bug?
Flags: needinfo?(blassey.bugs)
As Randall said, discovery is an error prone process. Chrome has similar issues with discovery so I think this can be closed out. If we have ideas on improving the discovery process, we can file bugs to investigate those.
Flags: needinfo?(blassey.bugs)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #65)
> As Randall said, discovery is an error prone process. Chrome has similar
> issues with discovery so I think this can be closed out. If we have ideas on
> improving the discovery process, we can file bugs to investigate those.

Thank you Brad! Closing this issue.
Status: RESOLVED → VERIFIED
Depends on: 1124880
Is there away to disable this in about:config if the user doesn't need it?
Depends on: 1158189
No longer depends on: 1124880
You need to log in before you can comment on or make changes to this bug.