Add Basic navigator.mediaDevices test. #2657
Conversation
|
Critic review: https://critic.hoppipolla.co.uk/r/6243 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
Reviewers for this pull request are: @alvestrand, @dontcallmedom, and @foolip. |
| <!doctype html> | ||
| <html> | ||
| <head> | ||
| <title>getUserMedia: test that getUserMedia is present (with or without vendor prefix)</title> |
There was a problem hiding this comment.
Ths test checks that enumerateDevices is present, while the title, file name and so on claim to check for getUserMedia. Can you make it consistent?
(It's fine to check both in the same test)
|
Seems to me you're doing an extremely deep hierarchy: mediacapture-streams/obtaining-local-multimedia-content/{navigatorusermedia,mediadevices}/. Any reason for this deepness? If so, can you explain proper placement in a README.md file in the mediacapture-streams directory? |
| @@ -1,3 +1,4 @@ | |||
| @alvestrand | |||
| @dontcallmedom | |||
| @foolip | |||
| @agouaillard | |||
There was a problem hiding this comment.
@alvestrand : we decided with dom to follow the chapters in the spec for easier spec <-> tests scrum'ing.
@foolip will do.
| <script> | ||
| "use strict"; | ||
| //NOTE ALEX: for completion, a test for ondevicechange event is missing. | ||
| test(function () { |
There was a problem hiding this comment.
No objection to having this kind of test, but isn't this (API presence) better done by adding the WebIDL-driven tests that we did earlier for peerconnection API?
(to be clear: I'm happy merging this version, but @agouaillard should look at the WebIDL-driven tools)
There was a problem hiding this comment.
You mean like the one used for media track API in mediastream-api/mediastreamtrack-init ?
|
I think we should merge this and progress iteratively. @dontcallmedom can decide. |
| "use strict"; | ||
| //NOTE ALEX: for completion, a test for ondevicechange event is missing. | ||
| test(function () { | ||
| assert_true(undefined !== navigator.mediaDevices.enumerateDevices(), "navigator.mediaDevices.enumerateDevices() exists"); |
There was a problem hiding this comment.
I think @foolip mentioned it before, but it seems to me more logical to test
undefined !== navigator.mediaDevices.enumerateDevicessince we're checking for existence
There was a problem hiding this comment.
You mean without the parenthesis? Please educate me on the difference in JS.
There was a problem hiding this comment.
ok, test existence, and not execute it. got it. fixing it.
| assert_equals(error.name, "SecurityError", "SecurityError returned"); | ||
| assert_equals(error.constraintName, null, "constraintName attribute not set for permission denied"); | ||
| assert_equals(error.name, "securityError", "securityError returned"); | ||
| // NOTE ALEX: this is also nullable, but practically, all the browsers but FF set it to an empty string. |
There was a problem hiding this comment.
we should test based on what the spec says, not based on what implementations do; if and when the spec gets proven to be wrong, we will update the test, but if we hide implementation bugs in our test cases, we won't find the discrepancies between spec and implementations.
There was a problem hiding this comment.
Agreed, and the commit you commented reverted to the original code that does just that as far as the error type is concerned. Now, some object are nullable by spec, but the spec does not address the default value at initialization. In that case, my understanding is that it is left to the implementation to decide if an empty string or a null object should be set. Do I understand wrong (and there is a spec that says that e.g. nullable object should be initialized as null)? Should we make the spec explicit in that case?
There was a problem hiding this comment.
The only error objects that would have a non-undefined "constraintName" would be from overconstraint event; for any other error, the correct test is to assert_equals(error.constraintName, undefined)
Add Basic navigator.mediaDevices test.
=> DOM