1088758 - Add the ability to mirror tabs from desktop to a second screen

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mirror.patch

Splitting out tab mirroring work from bug 1054959

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8511116 [details] [diff] [review]
mirror.patch

AFAICT this just adding a Tools menu item, which is probably not ideal (not discoverable at all). Need to work with UX on a better option for that.

On my machine the menu appears and just doesn't have anything in it. It needs to be either hidden or disabled (UX call).

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

>+function populateMirrorTabMenu(popup) {
>+  let videoEl = this.target;

Not used. Also should remove the reportError().

>+        let width = gBrowser.selectedBrowser.contentWindow.scrollWidth;
>+        let height = gBrowser.selectedBrowser.contentWindow.scrollHeight;

>+        app.mirror(function() {}, window, viewport, function() {}, gBrowser.selectedBrowser.contentWindow);

These don't look very e10s friendly?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mirror.patch

I don't have a roku available in Portland, so this is untested.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mirror.patch

This works, as tested with rbarker's roku simulator[1]

1] https://people.mozilla.org/~rbarker/roku-sim.tgz

Jesup, I changed our check for privileged callers in MediaManager. I suspect this is what we actually want here (testing the privilege level of the caller rather than the document), but this code dates back to the original implementation of GuM in bug 752352 and has since wound its way though a few refactors.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mirror.patch

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8534096 [details] [diff] [review]
mirror.patch

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

r- only because c++ callers of GetUserMedia() need to be modified to ensure they have a JS context (AutoNoJSAPI()) or the code needs to assume c++ callers are privileged in some manner (and be sure that's true).  SpeechAPI, and jib thinks some B2G callers call it directly.  If those are covered, r+

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: chrome_priv_check.patch

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mirror.patch

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8534411 [details] [diff] [review]
mirror.patch

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

r?mfinkle for RokuApp.jsm and SimpleServiceDiscovery.jsm, r?mconley for everything else

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8534411 [details] [diff] [review]
mirror.patch

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

>+addMessageListener("secondscreen:tab-mirror", function(message) {
>+    try {
>+        if (SimpleServiceDiscovery.numDevices() == 0) {

>+            SimpleServiceDiscovery.registerDevice(rokuDevice);
>+        }

I'd rather not add SimpleServiceDiscovery.numDevices(), but just call SimpleServiceDiscovery.registerDevice(...) which will ignore the device if it's already registered. If you don't like the log-spew, we could remove that.

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

>+  numDevices: function numDevices() {
>+    return this._devices.size;
>+  },

As mentioned above, I'd rather not add this.

r+ with the nits addressed

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Comment on attachment 8534411 [details] [diff] [review]
> mirror.patch
> 
> >diff --git a/browser/base/content/content.js b/browser/base/content/content.js
> 
> >+addMessageListener("secondscreen:tab-mirror", function(message) {
> >+    try {
> >+        if (SimpleServiceDiscovery.numDevices() == 0) {
> 
> >+            SimpleServiceDiscovery.registerDevice(rokuDevice);
> >+        }
> 
> I'd rather not add SimpleServiceDiscovery.numDevices(), but just call
> SimpleServiceDiscovery.registerDevice(...) which will ignore the device if
> it's already registered. If you don't like the log-spew, we could remove
> that.
My concern isn't log spew as much as we'll be calling this a lot, and for each mirroring end point we add (though currently we only have roku) and presumably checking the size of the map is O(1) while calling get is O(log(n)).
> 
> >diff --git a/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm b/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm
> 
> >+  numDevices: function numDevices() {
> >+    return this._devices.size;
> >+  },
> 
> As mentioned above, I'd rather not add this.
> 
> r+ with the nits addressed

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8534411 [details] [diff] [review]
mirror.patch

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

Just some minor suggestions - but in general, this looks good.

::: browser/base/content/browser.js
@@ +2989,5 @@
> +  let services = CastingApps.getServicesForMirroring();
> +  services.forEach(service => {
> +    let item = doc.createElement("menuitem");
> +    item.setAttribute("label", service.friendlyName);
> +    item.addEventListener("command", event => {

Instead of attaching a command event listener to each item, I recommend the following:

1) Having a single oncommand event handler set on the popup directly. That handler looks at the event target, extracts some unique service ID from it, retrieves the service for that unique ID, and then sends the message down to the selected browser.
2) In populateMirrorTabMenu, for each service, append a menuitem, and set an attribute on that menuitem to be the unique ID for the service. This is what the oncommand handler will read.

@@ +2990,5 @@
> +  services.forEach(service => {
> +    let item = doc.createElement("menuitem");
> +    item.setAttribute("label", service.friendlyName);
> +    item.addEventListener("command", event => {
> +      gBrowser.selectedBrowser.messageManager.sendAsyncMessage("secondscreen:tab-mirror",

Browser's been using Capitalized message strings. Might as well stick with it - SecondScreen:tab-mirror

::: browser/base/content/content.js
@@ +78,5 @@
>    docShell.mixedContentChannel = null;
>  });
>  
> +addMessageListener("secondscreen:tab-mirror", function(message) {
> +    try {

Nit - the rest of this file has 2-space indentation. Let's keep rolling with that.

@@ +92,5 @@
> +            types: ["video/mp4"],
> +            extensions: ["mp4"]
> +            };
> +
> +        // Register targets

Busted indentation

@@ +93,5 @@
> +            extensions: ["mp4"]
> +            };
> +
> +        // Register targets
> +            SimpleServiceDiscovery.registerDevice(rokuDevice);

rokuDevice actually doesn't seem to be used elsewhere, so maybe just:

SimpleServiceDiscovery.registerDevice({
  id: "roku:ecp",
  /* ... */
});

@@ +100,5 @@
> +        if (app) {
> +            let width = content.scrollWidth;
> +            let height = content.scrollHeight;
> +            let viewport = {cssWidth: width, cssHeight: height, width: width, height: height};
> +            let en = Services.wm.getXULWindowEnumerator(null);

This doesn't appear to be used.

@@ +103,5 @@
> +            let viewport = {cssWidth: width, cssHeight: height, width: width, height: height};
> +            let en = Services.wm.getXULWindowEnumerator(null);
> +            app.mirror(function() {}, content, viewport, function() {}, content);
> +        }
> +    } catch (ex) {Cu.reportError(ex);}

This massive try-catch freaks me out. What does it protect us from? If we throw when receiving this message from the parent... I don't see bad things happening.

::: browser/modules/CastingApps.jsm
@@ +134,5 @@
> +    let filteredServices = SimpleServiceDiscovery.services.filter(service => {
> +      return service.mirror;
> +    });
> +
> +    return filteredServices;

Might as well just do:

return SimpleServiceDiscovery.services.filter(service => service.mirror);

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mirror.patch

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8536841 [details] [diff] [review]
mirror.patch

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

Just some last suggestions, and then let's land this puppy.

(Note that I did not actually apply and test the patch - my review was strictly from inspection).

::: browser/base/content/browser.js
@@ +2976,5 @@
>  
> +function mirrorShow(popup) {
> +  let services = CastingApps.getServicesForMirroring();
> +  if (services.length == 0) {
> +    popup.ownerDocument.getElementById("menu_mirrorTabCmd").disabled = true;

popup.ownerDocument.getElementById("menu_mirrorTabCmd").disabled = !Services.length;

@@ +2984,5 @@
> +}
> +
> +function mirrorMenuItemClicked() {
> +  gBrowser.selectedBrowser.messageManager.sendAsyncMessage("SecondScreen:tab-mirror",
> +                                                              {service: this._service});

Nit - indentation

Also, we generally use the event.originalTarget - so take an event argument to mirrorMenuItemClicked, and pass event.originalTarget._service.

@@ +2996,5 @@
> +  services.forEach(service => {
> +    let item = doc.createElement("menuitem");
> +    item.setAttribute("label", service.friendlyName);
> +    item._service = service;
> +    item.addEventListener("command", mirrorMenuItemClicked, false);

capturing is false by default, so no need for the third parameter.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0be7c6566ae
https://hg.mozilla.org/mozilla-central/rev/a7770ec46f04

Comment 15

[:Gijs (he/him)]

Reading comment #1), who made the UX decision to have an always-present disabled menu instead of a hidden one, and where? 

I don't think this should be always-visible, considering most users don't actually have a device with which they can mirror. We can show them if/when they do.

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to :Gijs Kruitbosch from comment #15)
> Reading comment #1), who made the UX decision to have an always-present
> disabled menu instead of a hidden one, and where? 
no one

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs Kruitbosch from comment #15)
> Reading comment #1), who made the UX decision to have an always-present
> disabled menu instead of a hidden one, and where? 
> 
> I don't think this should be always-visible, considering most users don't
> actually have a device with which they can mirror. We can show them if/when
> they do.

I agree. Let's file a follow-up bug to get that menuitem hidden in those cases.