Closed Bug 1041658 Opened 6 years ago Closed 6 years ago

write tests for the global webrtc sharing indicator

Categories

(Firefox :: Site Permissions, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 34
Iteration:
34.3
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(1 file)

Bug 1037408 landed without tests because we were wanted to land before the Firefox 33 string freeze and were out of time. We should still write the tests.
Flags: firefox-backlog+
Blocks: 1041660
No longer blocks: 1041660
Hi Florian, can you mark this bug as either [qa+] or [qa-] for verification.
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 34.1
QA Whiteboard: [qa?]
Flags: needinfo?(florian)
QA Whiteboard: [qa?] → [qa-]
Flags: needinfo?(florian)
Blocks: 1040061
QA Contact: drno
Whiteboard: [sceensharing-uplift]
I'm a little confused here: if I'm assigned as QA for a ticket for writing tests, does that mean the expectation is that I'll be writing the test, or the assigned person writes the test and the QA contact verifies the tests are working? :-)
(In reply to Nils Ohlmeier [:drno] from comment #2)
> I'm a little confused here: if I'm assigned as QA for a ticket for writing
> tests, does that mean the expectation is that I'll be writing the test, or
> the assigned person writes the test and the QA contact verifies the tests
> are working? :-)

I think you were assigned as QA by mistake, [qa-] in the QA whiteboard indicates that QA isn't needed here.
QA Contact really just means that you're the person responsible for testing. If the bug doesn't need testing, as indicated by [qa-] then there's no action needed on your part. However I still think it's useful to keep the QA contact assigned in case something comes up that invalidates the [qa-] or in case follow-up questions arise that need input from QA. 

It just saves people from having to track someone down.
Iteration: 34.1 → 34.2
Assignee: florian → nobody
Status: ASSIGNED → NEW
Iteration: 34.2 → ---
Florian -- Are we going to uplift this to Fx33?  I believe we are NOT (based on our irc conversation).  So, are you ok with removing "[sceensharing-uplift]" from the whiteboard?
Flags: needinfo?(florian)
(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #5)
> Florian -- Are we going to uplift this to Fx33?

I think we will, yes. Changes here will be test-only.

> I believe we are NOT (based on our irc conversation).

The bug I mentioned during our IRC conversation was bug 1041660.
Flags: needinfo?(florian)
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Attached patch PatchSplinter Review
Requesting review as the test passes locally for me on both Mac and Windows, and a previous try run was green, but you may want to wait for the results from try for this exact patch (https://tbpl.mozilla.org/?tree=Try&rev=04f3ac66f1c8) before reviewing.
Attachment #8477421 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8477421 [details] [diff] [review]
Patch

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

::: browser/base/content/test/general/browser_devices_get_user_media.js
@@ +769,5 @@
> +        gWebRTCUI.showSharingDoorhanger(activeStreams[0], "Devices");
> +      }
> +      else {
> +        let win =
> +          Services.wm.getEnumerator("Browser:WebRTCGlobalIndicator").getNext();

Nit:

let win = Services.wm.getMostRecentWindow("...");

::: browser/base/content/test/general/head.js
@@ +574,5 @@
>    let expectedState = expected ? "visible" : "hidden";
>    let msg = "WebRTC indicator " + expectedState;
> +  is(ui.showGlobalIndicator, !!expected, msg);
> +
> +  let expectVideo = false, expectAudio = false, expectScreen = false;

Nit: replace 578-586 with:

let {video: expectVideo, audio: expectAudio, screen: expectScreen} = expected || {};

then you could add the same !! to the 3 is() calls afterwards, but I think it's not even necessary, as is() doesn't use strict equals.

@@ +600,5 @@
> +    let hasWindow = indicator.hasMoreElements();
> +    is(hasWindow, !!expected, "popup " + msg);
> +    if (hasWindow) {
> +      let win = indicator.getNext();
> +      let elt = win.document.documentElement;

Perhaps for the following 3 is():

for (let item of ["video", "audio", "screen"]) {
  is(elt.getAttribute("sharing" + item), (expected && expected[item]) ? "true" : "",
     item + "global indicator attribute as expected");
}

It's up to you.
Attachment #8477421 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks for the review!

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

> Nit: replace 578-586 with:
> 
> let {video: expectVideo, audio: expectAudio, screen: expectScreen} =
> expected || {};

This would cause "reference to undefined property expected.screen" JS strict warnings.


I don't have my Windows machine with me today, so I pushed to try again to ensure I didn't break anything while addressing the other 2 comments: https://tbpl.mozilla.org/?tree=Try&rev=9374d045164a
(In reply to Florian Quèze [:florian] [:flo] from comment #9)
> Thanks for the review!
> 
> (In reply to :Gijs Kruitbosch from comment #8)
> 
> > Nit: replace 578-586 with:
> > 
> > let {video: expectVideo, audio: expectAudio, screen: expectScreen} =
> > expected || {};
> 
> This would cause "reference to undefined property expected.screen" JS strict
> warnings.

I think we should kill the JS strict warning implementation of this type with fire.

In the meantime, I guess you could use some variation of this or leave things as they are, if you prefer.
(In reply to :Gijs Kruitbosch from comment #10)

> > This would cause "reference to undefined property expected.screen" JS strict
> > warnings.
> 
> I think we should kill the JS strict warning implementation of this type
> with fire.

I actually find this warning quite useful, several times it saved me a lot of time by identifying quickly typos (typically a letter having the wrong case) that could have taken a while to debug otherwise.

There may be a few edge cases where the warning should be disabled.
https://hg.mozilla.org/mozilla-central/rev/a66207560239
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Comment on attachment 8477421 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1037408
[User impact if declined]: none. This patch is only adding tests.
[Describe test coverage new/current, TBPL]: none before the patch.
[Risks and why]: low, if something goes wrong tbpl will tell us.
[String/UUID change made/needed]: none.
Attachment #8477421 - Flags: approval-mozilla-aurora?
Attachment #8477421 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing around bug 1043372 (or an uplift nomination on that bug too).
Flags: needinfo?(florian)
Bug 1043372 is not upliftable, it adds new strings. I'll prepare a branch patch.
Depends on: 1060315
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #15)
> Needs rebasing around bug 1043372 (or an uplift nomination on that bug too).

I pushed to try an adapted version of the patch along with the other screensharing patches that got aurora approval today: https://tbpl.mozilla.org/?tree=Try&rev=4e3a5c0f69b8

It's green, but the tree is currently closed.
Flags: needinfo?(florian)
Whiteboard: [sceensharing-uplift]
Component: General → Device Permissions
You need to log in before you can comment on or make changes to this bug.