1284877 - show a preview when the user selects a screen/window/application to share

Status: VERIFIED FIXED

(Firefox :: Site Permissions, defect, P2)

Description

[Florian Quèze [:florian]]

As part of the screensharing whitelist removal work, we want to implement this new permission UI that shows a live preview of the window the user is about to share: https://mozilla.invisionapp.com/share/AF71R266U#/screens/152725629

Comment 1

[Erin Lancaster [:elan]]

P1 when we are unblocked.

Comment 2

[Florian Quèze [:florian]]

Attachment: WIP v1

WIP, but most of the way there :-).

What remains to do:
- cleanup streams when the panel is closed / when selecting a different window
- maybe some theming polish, need to look again at the mockup
- see if some of this can be covered by automated tests.

If you want to try this, you need to apply first the r-'ed patch from bug 1284680.

Comment 3

[Florian Quèze [:florian]]

Attachment: WIP v2

This doesn't leave streams behind anymore, and includes the hack we decided to use in bug 1284680.

Comment 4

[Florian Quèze [:florian]]

I filed bug 1313324 to ensure this is well covered by tests. The reason for not handling testing in this bug is that this needs to land before bug 1284878, which will contain a couple localized strings. We want to land the screensharing whitelist removal before 52 merges to aurora, so that we don't have to support the whitelist for all of the next ESR cycle, which will be based on 52. The time before 52 merges to aurora is short at this point, and tests is something we can uplift, so I think it makes sense to prioritize landing all the UI pieces first. I'll get the test coverage sorted out before 52 reaches beta.

Comment 5

[Florian Quèze [:florian]]

Attachment: Patch v3

Removed a leftover debugging dump. I think this is ready for review, as I'll handle testing in a separate bug, as explained in comment 4.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8805070 [details] [diff] [review]
Patch v3

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

::: browser/modules/ContentWebRTC.jsm
@@ +292,5 @@
>    }
>  
>    for (let contentWindow of contentWindows) {
> +    if (contentWindow.document.documentURI == kBrowserURL) {
> +      // There may be a preview showed at the same time as other streams.

Nit: shown

::: browser/modules/webrtcUI.jsm
@@ +375,5 @@
>        }
>  
> +      // Clean-up video streams of screensharing previews.
> +      if ((aTopic == "dismissed" || aTopic == "removed") &&
> +          requestTypes.indexOf("Screen") != -1) {

requestTypes.includes("Screen")

@@ +511,5 @@
>          chromeDoc.getElementById("webRTC-selectWindow-menulist").removeAttribute("value");
>          chromeDoc.getElementById("webRTC-all-windows-shared").hidden = true;
> +        if (menupopup._commandEventListener)
> +          menupopup.removeEventListener("command", menupopup._commandEventListener);
> +        menupopup._commandEventListener = event => {

How is this removed if prompt() is not called again? Looks like the menupopup stays in the DOM, but really this listener should go away when the overall prompt disappears (e.g. in the same handler where you 'clean up' the video), or its references in this app-global JSM might leak the window in question...

@@ +533,5 @@
> +
> +          let constraints = { video: { mediaSource: type, deviceId: {exact: deviceId } } };
> +          let chromeWin = chromeDoc.defaultView;
> +          chromeWin.navigator.mediaDevices.getUserMedia(constraints).then(stream => {
> +            video.src = chromeWin.URL.createObjectURL(stream);

This races - the event listener could have been called again by now, for a different item.

::: browser/themes/shared/notification-icons.inc.css
@@ +154,5 @@
>  .screen-icon.blocked-permission-icon {
>    list-style-image: url(chrome://browser/skin/notification-icons.svg#screen-blocked);
>  }
>  
> +html|*#webRTC-previewVideo {

Do you need the html| for this to work? I would have expected just the ID selector to be fine, and for you to only need to namespace the tagname, which isn't relevant here.

@@ +158,5 @@
> +html|*#webRTC-previewVideo {
> +  width: 300px;
> +  /* If we don't set the min-width, width is ignored. */
> +  min-width: 300px;
> +  max-height: 200px;

Will this warp if used with a screen/window that is higher than it is wide ("portrait"), as you're forcing the width to be big and the height to be small?

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch v4

(In reply to :Gijs Kruitbosch from comment #6)

> ::: browser/modules/webrtcUI.jsm
> @@ +375,5 @@
> >        }
> >
> > +      // Clean-up video streams of screensharing previews.
> > +      if ((aTopic == "dismissed" || aTopic == "removed") &&
> > +          requestTypes.indexOf("Screen") != -1) {
>
> requestTypes.includes("Screen")

The reason why I used indexOf was that there's another indexOf call a few lines above, and I like consistency. I've now changed all 3 occurrences to use includes instead.

> @@ +511,5 @@
> >          chromeDoc.getElementById("webRTC-selectWindow-menulist").removeAttribute("value");
> >          chromeDoc.getElementById("webRTC-all-windows-shared").hidden = true;
> > +        if (menupopup._commandEventListener)
> > +          menupopup.removeEventListener("command", menupopup._commandEventListener);
> > +        menupopup._commandEventListener = event => {
>
> How is this removed if prompt() is not called again? Looks like the
> menupopup stays in the DOM, but really this listener should go away when the
> overall prompt disappears (e.g. in the same handler where you 'clean up' the
> video), or its references in this app-global JSM might leak the window in
> question...

The reference is attached to the menupopup element which is in the same window, so unless there are implicit references (eg. due to the function being created from a different compartment), I don't think this would leak... I changed it anyway, as it would be easy in the future to add references without noticing the effect.

> @@ +533,5 @@
> > +
> > +          let constraints = { video: { mediaSource: type, deviceId: {exact: deviceId } } };
> > +          let chromeWin = chromeDoc.defaultView;
> > +          chromeWin.navigator.mediaDevices.getUserMedia(constraints).then(stream => {
> > +            video.src = chromeWin.URL.createObjectURL(stream);
>
> This races - the event listener could have been called again by now, for a
> different item.

I thought about this at the time and decided it wasn't worth complicating the code to address this unlikely edge case, as getUserMedia would return results in the same order as the calls and all the race would cause is a stream starting for a short amount of time before it gets replaced by another one. But thinking about this some more, there was another race that worried me more: if the panel is closed before getUserMedia completed, the cleanup code doesn't run and we leave an active stream until the browser window is closed (or the panel used again). The new version of the patch fixes both issues at once.

> ::: browser/themes/shared/notification-icons.inc.css
> @@ +154,5 @@
> >  .screen-icon.blocked-permission-icon {
> >    list-style-image: url(chrome://browser/skin/notification-icons.svg#screen-blocked);
> >  }
> >
> > +html|*#webRTC-previewVideo {
>
> Do you need the html| for this to work? I would have expected just the ID
> selector to be fine, and for you to only need to namespace the tagname,
> which isn't relevant here.

Yes, I do. And like you, I also initially thought I only needed to specify the namespace for tag names.

> @@ +158,5 @@
> > +html|*#webRTC-previewVideo {
> > +  width: 300px;
> > +  /* If we don't set the min-width, width is ignored. */
> > +  min-width: 300px;
> > +  max-height: 200px;
>
> Will this warp if used with a screen/window that is higher than it is wide
> ("portrait"), as you're forcing the width to be big and the height to be
> small?

The aspect ratio will be preserved, and empty areas will appear on the sides. I worked on this part of the CSS some more for bug 1284878 to ensure there's always enough space to display the warning.

Comment 8

[:Gijs (he/him)]

Thanks for the thorough explanations!

Comment 9

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c8208b0139c97d9470ea64251884acfcc6ac82
Bug 1284877 - show a preview when the user selects a screen/window/application to share, r=Gijs.

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f5c8208b0139

Comment 12

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52b2 Win 7, Ubuntu 16.04, OS X 10.11.