-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use webdriver to set permission on getUserMedia calls #31847
Use webdriver to set permission on getUserMedia calls #31847
Conversation
ebecf02
to
93e8800
Compare
Rely instead of SetPermission webdriver hook; this allows testing permission denial.
93e8800
to
1f8ee12
Compare
5eac0e6
to
125fa16
Compare
Rewrite mediacapture-streams using async/await for consistency in the process Move a couple of manual tests back to automated as a result
125fa16
to
ea93b94
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this change seems reasonable but I only took a close look at mediacapture-image/ImageCapture-creation.https.html
since that's the file I'm an owner for.
@@ -5,6 +5,9 @@ | |||
<link rel="help" href="https://w3c.github.io/mediacapture-record/MediaRecorder.html"> | |||
<script src="/resources/testharness.js"></script> | |||
<script src="/resources/testharnessreport.js"></script> | |||
<script src=/resources/testdriver.js></script> | |||
<script src=/resources/testdriver-vendor.js></script> | |||
<script src='../mediacapture-streams/permission-helper.js'></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we please have one convention on how filenames are quoted in "script src=" statements?
This makes 3 different versions in a single file.
<html> | ||
<head> | ||
<title>Test that the HTMLMediaElement preload 'none' attribute value is ignored for MediaStream used as srcObject and MediaStream object URLs used as src.</title> | ||
<link rel="author" title="Matthew Wolenetz" href="mailto:wolenetz@chromium.org"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file seems like it belongs to a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that file was a -manual
file, presumably because when it was created, one would not gain access to getUserMedia. Now that we do, it feels reasonable to make the move here.
for (let s of scope) { | ||
await test_driver.set_permission({ name: s }, status, true); | ||
} | ||
} catch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generic catch is usually a bad idea. What error do we expect here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to deal with browsers that don't implement the set_permission
webdriver command (i.e. both Firefox and Safari at this time point).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we should catch "no such function". I'm worried about the case where it fails for an unrelated reason (such as calling with "scope=['cammera']" and getting a "no such value" exception - that could be hard to debug when the error is caught and ignored).
@@ -0,0 +1,10 @@ | |||
async function setPermission(status="granted", scope=["camera", "microphone"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "status" argument is never used. Is it a future goal to be able to set setPermission(status=denied, scope=["camera"])? If so, document (or even better, implement).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's used in GUM-deny.html
So the harness-setting-permission function has finally landed? Great! A couple of things:
|
Will fix to
I can take a pass at setting permissions more specifically.
I sure wish it was; but doing this cleanly would require using JS modules, which I don't think the WPT infrastructure is ready for at this point.
It does remove the flag, see https://github.com/web-platform-tests/wpt/pull/31847/files#diff-0df24b5b583c460182e687f7dc7a6a79dd2cd3389bc4a96f48483f60fceb51f7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great step forward in testing permission-gated functionality!
13a254a
to
328aa26
Compare
…erMedia calls , a=testonly Automatic update from web-platform-tests Use webdriver to set permission on getUserMedia calls (#31847) * Remove fake-ui from chrome wptrunner Rely instead of SetPermission webdriver hook; this allows testing permission denial. * Use webdriver to set permission on getUserMedia calls Rewrite mediacapture-streams using async/await for consistency in the process Move a couple of manual tests back to automated as a result * Catch web driver error more specifically when setting permission per web-platform-tests/wpt#31847 (review) * Rename setPermission in setMediaPermission per web-platform-tests/wpt#31847 (comment) * Limit media capture permissions to what is strictly needed for the test per web-platform-tests/wpt#31847 (comment) * Align quoting styles for script loading -- wpt-commits: 319a0a7580aaee957801545901150a0dabd9e735 wpt-pr: 31847
this includes removing the default fake-ui from chrome wpt runner - this makes obtaining permission a prerequisite for using
getUserMedia
in tests.