1041658 - write tests for the global webrtc sharing indicator

Status: RESOLVED FIXED

(Firefox :: Site Permissions, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Marco Mucci [:MarcoM]]

Hi Florian, can you mark this bug as either [qa+] or [qa-] for verification.

Comment 2

[Nils Ohlmeier [:drno]]

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? :-)

Comment 3

[Florian Quèze [:florian]]

(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.

Comment 4

[u279076]

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.

Comment 5

[Maire Reavy [:mreavy]]

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?

Comment 6

[Florian Quèze [: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.

Comment 7

[Florian Quèze [:florian]]

Attachment: Patch

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.

Comment 8

[:Gijs (he/him)]

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.

Comment 9

[Florian Quèze [:florian]]

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

Comment 10

[:Gijs (he/him)]

(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.

Comment 11

[Florian Quèze [:florian]]

(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.

Comment 12

[Florian Quèze [:florian]]

Try (from comment 9) was green.

https://hg.mozilla.org/integration/fx-team/rev/a66207560239

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Florian Quèze [:florian]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing around bug 1043372 (or an uplift nomination on that bug too).

Comment 16

[Florian Quèze [:florian]]

Bug 1043372 is not upliftable, it adds new strings. I'll prepare a branch patch.

Comment 17

[Florian Quèze [:florian]]

(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.

Comment 18

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ec4d34c39a5