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

VERIFIED FIXED in Firefox 52

Status

()

defect
P2
normal
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
Firefox 52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 verified)

Details

(Whiteboard: [fxprivacy])

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

3 years ago
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
Assignee

Updated

3 years ago
Depends on: 1284909
Assignee

Updated

3 years ago
Depends on: 1284680, 1284683
P1 when we are unblocked.
Priority: -- → P2
Whiteboard: [fxprivacy][triage] → [fxprivacy]
Flags: qe-verify?
Assignee

Updated

3 years ago
Assignee: nobody → florian
Flags: qe-verify? → qe-verify+
Assignee

Comment 2

3 years ago
Posted patch WIP v1 (obsolete) — Splinter Review
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.
Assignee

Comment 3

3 years ago
Posted patch WIP v2 (obsolete) — Splinter Review
This doesn't leave streams behind anymore, and includes the hack we decided to use in bug 1284680.
Assignee

Updated

3 years ago
Attachment #8794223 - Attachment is obsolete: true
Assignee

Comment 4

3 years ago
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.
Assignee

Comment 5

3 years ago
Posted patch Patch v3 (obsolete) — Splinter Review
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.
Attachment #8805070 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

3 years ago
Attachment #8804812 - Attachment is obsolete: true
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?
Attachment #8805070 - Flags: review?(gijskruitbosch+bugs) → review-
Assignee

Comment 7

3 years ago
Posted patch Patch v4Splinter Review
(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.
Attachment #8806832 - Flags: review?(gijskruitbosch+bugs)
Assignee

Updated

3 years ago
Attachment #8805070 - Attachment is obsolete: true
Attachment #8806832 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks for the thorough explanations!
Assignee

Comment 9

3 years ago
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.
Status: NEW → ASSIGNED

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5c8208b0139
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52

Updated

3 years ago
Depends on: 1316465
Duplicate of this bug: 1329496
Depends on: 1336066
Depends on: 1336067
Verified fixed FX 52b2 Win 7, Ubuntu 16.04, OS X 10.11.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.