Closed
Bug 1041658
Opened 10 years ago
Closed 10 years ago
write tests for the global webrtc sharing indicator
Categories
(Firefox :: Site Permissions, defect)
Firefox
Site Permissions
Tracking
()
People
(Reporter: florian, Assigned: florian)
References
Details
Attachments
(1 file)
7.64 KB,
patch
|
Gijs
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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+
Comment 1•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
QA Whiteboard: [qa?] → [qa-]
Flags: needinfo?(florian)
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Comment 2•10 years ago
|
||
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? :-)
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Updated•10 years ago
|
Iteration: 34.1 → 34.2
Updated•10 years ago
|
Assignee: florian → nobody
Status: ASSIGNED → NEW
Iteration: 34.2 → ---
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
(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)
Updated•10 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Iteration: --- → 34.3
QA Whiteboard: [qa-]
Flags: qe-verify-
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
||
(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.
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Assignee | ||
Comment 12•10 years ago
|
||
Try (from comment 9) was green.
https://hg.mozilla.org/integration/fx-team/rev/a66207560239
Comment 13•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Assignee | ||
Comment 14•10 years ago
|
||
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?
Updated•10 years ago
|
status-firefox33:
--- → affected
status-firefox34:
--- → fixed
Updated•10 years ago
|
Attachment #8477421 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•10 years ago
|
||
Needs rebasing around bug 1043372 (or an uplift nomination on that bug too).
Flags: needinfo?(florian)
Keywords: branch-patch-needed
Assignee | ||
Comment 16•10 years ago
|
||
Bug 1043372 is not upliftable, it adds new strings. I'll prepare a branch patch.
Assignee | ||
Comment 17•10 years ago
|
||
(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)
Assignee | ||
Comment 18•10 years ago
|
||
Keywords: branch-patch-needed
Updated•10 years ago
|
Whiteboard: [sceensharing-uplift]
Assignee | ||
Updated•10 years ago
|
Component: General → Device Permissions
You need to log in
before you can comment on or make changes to this bug.
Description
•