Closed Bug 1348174 Opened 8 years ago Closed 8 years ago

Test origin-unique deviceId persistence.

Categories

(Core :: WebRTC: Audio/Video, enhancement, P1)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: jib, Assigned: jib)

References

Details

Attachments

(2 files)

Bug 1348174 - Convert test_enumerateDevices.html to async/await.
59 bytes, text/x-review-board-request
ng
: review+
Details
Bug 1348174 - Test that deviceIds are stable for same origin and differ across origins.
59 bytes, text/x-review-board-request
jesup
: review+
Details
Add missing tests to catch e.g. bug 1340163. Test that deviceIds are stable for same origin and differ across origins.
Comment on attachment 8848334 [details] Bug 1348174 - Test that deviceIds are stable for same origin and differ across origins. https://reviewboard.mozilla.org/r/121244/#review123330
Attachment #8848334 - Flags: review?(rjesup) → review+
Comment on attachment 8848333 [details] Bug 1348174 - Convert test_enumerateDevices.html to async/await. https://reviewboard.mozilla.org/r/121242/#review123334 ::: dom/media/tests/mochitest/test_enumerateDevices.html:29 (Diff revision 1) > - return f().then(() => ok(false, msg + " must fail"), e => { > + try { > + await f(); > + ok(false, msg + " must fail"); > + } catch(e) { > - is(e.name, reason, msg + " must fail: " + e.message); > + is(e.name, reason, msg + " must fail: " + e.message); > - if (constraint) { > + if (constraint) { A constraint seems to always be supplied, the conditional can be removed. ::: dom/media/tests/mochitest/test_enumerateDevices.html:31 (Diff revision 1) > + ok(false, msg + " must fail"); > + } catch(e) { > - is(e.name, reason, msg + " must fail: " + e.message); > + is(e.name, reason, msg + " must fail: " + e.message); > - if (constraint) { > + if (constraint) { > - is(e.constraint, constraint, msg + " must fail w/correct constraint."); > + is(e.constraint, constraint, msg + " must fail w/correct constraint."); > - } > + } I would lean towards rethrowing e, if both the constraint is provided, and e doesn't match constraint, so SyntaxErrors and the like bubble up. WDYT? ::: dom/media/tests/mochitest/test_enumerateDevices.html:38 (Diff revision 1) > +} > > var pushPrefs = (...p) => SpecialPowers.pushPrefEnv({set: p}); > +var gUM = c => navigator.mediaDevices.getUserMedia(c); > + > +var validateDevice = ({kind, label, deviceId, groupId}) => { Snazzy destructuring. ::: dom/media/tests/mochitest/test_enumerateDevices.html:42 (Diff revision 1) > + > +var validateDevice = ({kind, label, deviceId, groupId}) => { > + ok(kind == "videoinput" || kind == "audioinput", "Known device kind"); > + is(deviceId.length, 44, "deviceId length id as expected for Firefox"); > + ok(label.length !== undefined, "Device label: " + label); > + todo_isnot(groupId, "", "groupId must be present."); Needs a comment with bug # optimally. ::: dom/media/tests/mochitest/test_enumerateDevices.html:45 (Diff revision 1) > + is(deviceId.length, 44, "deviceId length id as expected for Firefox"); > + ok(label.length !== undefined, "Device label: " + label); > + todo_isnot(groupId, "", "groupId must be present."); > +} > + > +runTest(async () => { await runTest? ::: dom/media/tests/mochitest/test_enumerateDevices.html:52 (Diff revision 1) > -runTest(() => > - pushPrefs(["media.navigator.streams.fake", true]) > - .then(() => navigator.mediaDevices.enumerateDevices()) > + // Validate enumerated devices. > + > + let devices = await navigator.mediaDevices.enumerateDevices(); > - .then(devices => { > - ok(devices.length > 0, "At least one device found"); > + ok(devices.length > 0, "At least one device found"); > - var jsoned = JSON.parse(JSON.stringify(devices)); > + var jsoned = JSON.parse(JSON.stringify(devices)); Not part of your edit, but var should be let. ::: dom/media/tests/mochitest/test_enumerateDevices.html:59 (Diff revision 1) > - is(jsoned[0].deviceId, devices[0].deviceId, "deviceId survived serializer"); > + is(jsoned[0].deviceId, devices[0].deviceId, "deviceId survived serializer"); > - return devices.reduce((p, d) => { > - ok(d.kind == "videoinput" || d.kind == "audioinput", "Known device kind"); > - is(d.deviceId.length, 44, "Correct device id length"); > - ok(d.label.length !== undefined, "Device label: " + d.label); > - is(d.groupId, "", "Don't support groupId yet"); > + for (let device of devices) { > + validateDevice(device); > + // Test deviceId constraint > + let deviceId = device.deviceId; > + let constraints = (device.kind == "videoinput")? { video: { deviceId } } space between ')' and '?'
Comment on attachment 8848333 [details] Bug 1348174 - Convert test_enumerateDevices.html to async/await. https://reviewboard.mozilla.org/r/121242/#review123334 r+ with nits
Comment on attachment 8848333 [details] Bug 1348174 - Convert test_enumerateDevices.html to async/await. https://reviewboard.mozilla.org/r/121242/#review123394
Attachment #8848333 - Flags: review?(na-g) → review+
Comment on attachment 8848333 [details] Bug 1348174 - Convert test_enumerateDevices.html to async/await. https://reviewboard.mozilla.org/r/121242/#review123334 > A constraint seems to always be supplied, the conditional can be removed. Unaltered code, so I'm going to say not this patch. FWIW I'd prefer to keep the couple of recurring copies (a few more and we might move it centrally) of `mustFailWith` [1] general, for future reuse. Also, OverconstrainedError is the only error with a constraint member, so a specialization would look different: `mustFailWithOverconstrainedError` and wouldn't even take a `reason` argument. I think that's a case against over-specializing here. [1] https://dxr.mozilla.org/mozilla-central/search?q=mustFailWith&case=true&=mozilla-central > I would lean towards rethrowing e, if both the constraint is provided, and e doesn't match constraint, so SyntaxErrors and the like bubble up. WDYT? Unaltered code. Also, by design this function tests a doomed operation, so it can resume in the face of any error. Unexpected errors get logged as: failed | unknown exact deviceId on video must fail: "" is not a function - got "TypeError", expected "OverconstrainedError" > Needs a comment with bug # optimally. Added. > await runTest? Can't use `await` in top scope. Also, runTest reports its own errors, so we generally don't need to check for failure, nor do we generally need to do anything after it's done.
Added feedback and trying to get try to work.
Attachment #8848334 - Flags: review+ → review?(rjesup)
Forgot to hg add a file. Jesup can you take another look?
Note, I set r? from bugzilla only, as mozReview seems to have no workflow for re-review.
Flags: needinfo?(rjesup)
Comment on attachment 8848334 [details] Bug 1348174 - Test that deviceIds are stable for same origin and differ across origins. https://reviewboard.mozilla.org/r/121244/#review123632 lgtm
Attachment #8848334 - Flags: review?(rjesup) → review+
Flags: needinfo?(rjesup)
Pushed by jbruaroey@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/59f63f2a1dc8 Convert test_enumerateDevices.html to async/await. r=ng https://hg.mozilla.org/integration/autoland/rev/f60a3d32a6d4 Test that deviceIds are stable for same origin and differ across origins. r=jesup
https://hg.mozilla.org/mozilla-central/rev/59f63f2a1dc8 https://hg.mozilla.org/mozilla-central/rev/f60a3d32a6d4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1348871
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: