Closed Bug 1313324 Opened 3 years ago Closed 3 years ago

Cover the screensharing UI with browser chrome tests


(Firefox :: Site Permissions, defect, P3)




Firefox 53
Tracking Status
firefox53 --- fixed


(Reporter: florian, Assigned: florian)


(Depends on 2 open bugs)


(Whiteboard: [fxprivacy])


(1 file, 3 obsolete files)

The initial screensharing UI landed without any test. We are now polishing this UI with bug 1284877 and bug 1284878, and more importantly we are planning to remove the screensharing whitelist to make the feature available to all websites (bug 1127522). As part of this project I think we need to ensure the UI is well covered by tests.
Attached patch WIP v1 (obsolete) — Splinter Review
Assignee: nobody → florian
Whiteboard: [fxprivacy]
Depends on: 1037438
Attached patch Patch v2 (obsolete) — Splinter Review
I think I'm done, but I'll wait to see the color of try results before requesting review.
Attachment #8811839 - Attachment is obsolete: true
Depends on: 1320754
Attached patch Patch v3 (obsolete) — Splinter Review
Attachment #8814486 - Attachment is obsolete: true
Depends on: 1320994
Attached patch Patch v4Splinter Review
The colors I see on the try push from comment 19 are promising at this point, so I think it's time to request review. Panos, I picked you but feel free to redirect to whoever has cycles.

Before landing I intend to file bugs for the AppConstants.platform checks, to mention the bug numbers in the todo() comments.
Attachment #8816305 - Flags: review?(past)
Attachment #8815315 - Attachment is obsolete: true
Comment on attachment 8816305 [details] [diff] [review]
Patch v4

Review of attachment 8816305 [details] [diff] [review]:

::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js
@@ +387,5 @@
> +    function* check(expected = {}) {
> +      let shared = Object.keys(expected).join(" and ");
> +      if (shared) {
> +        Assert.deepEqual((yield getMediaCaptureState()), expected,
> +                         "expected nothing to be shared");

"nothing" wouldn't be accurate when |expected| has a non-default value, right?

::: browser/base/content/test/webrtc/head.js
@@ +247,5 @@
> +  return new Promise((resolve, reject) => {
> +    let mm = _mm();
> +    mm.addMessageListener("Test:MessageReceived", function listener({data}) {
> +      resolve(data);
> +      mm.removeMessageListener("Test:MessageReceived", listener);

Nit: removing the listener before further action is the more common pattern to avoid inadvertent reentrancy. Is there a particular reason you want it like this here?
Attachment #8816305 - Flags: review?(past) → review+
Depends on: 1323481
Depends on: 1323490
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Depends on: 1323880
You need to log in before you can comment on or make changes to this bug.