Open squapp opened 7 years ago
jdsmith3000
commented
7 years ago Do I understand correctly that the main benefit of this change is completeness of what's failing? As in, it saves fixing the first syntax fail today, only to discover another, and another...?
If so, this feels like a preferred test implementation; but timing for it is tight. It might belong in VNext.
kilikkuo
commented
7 years ago Hi, I'm not sure if another issue should be created for my concern. So I'll just leave my question below, hope to get a response. Please let me know if I'm asking in a wrong procedure or place.
Regarding the syntax check against MediaKeySession.load, currently EME spec just mentioned that the Session ID should be sanitized (at a minimum, to check that the length and value are reasonable), but spec doesn't explicitly point out the exact length. AFAIK, certain CDM returns session id in byte array which can be interpreted as string, i.e. "32", "33", "34". Then I saw these tests are now included in test cases.
I don't know the exact causes of failure for Chrom/Edige in less-than-2.html, but for FF, the length of session Id is not considered now.
I'm wondering if the following two cases should be removed for now ? A. mk4.createSession('temporary').load(1); B. mk6.createSession('temporary').load('1234'); And then adding notes in EME spec, e.g. *"Special characters, i.e. '!@#$%^&()' should be avoided "** ?
ddorwin
commented
7 years ago The sessionId parameter and attribute are defined as a DOMString. Other types should be a TypeError.
For the string parameters, the TypeError probably occurs because temporary sessions, which are the only ones supported by Clear Key implementations, do not support persistent session types:
If the result of running the Is persistent session type? algorithm on this object's session type is false, return a promise rejected with a newly created TypeError.
We should probably duplicate these tests to also use a persistent session type. The tests that pass DOMString would either result in a TypeError or resolved promise with false for not found. Since there are currently no UA implementations that would pass the Clear Key versions, we should make them separate tests so they can be easily ignored. Chrome OS should handle the drm- version of these tests correctly, though.
in addition to #3874 it might be worthwhile adding additional syntax tests: Right now syntax tests test the creation of MediaKeySystemAccess, MediaKeys and MediaKeySession objects with various valid (object is created) and invalid (exception thrown) arguments. In case of valid arguments, the object is created and members of this object are checked. If only one of the members is missing for the object, the test will fail, because the object isn't syntactically correct. For clearer test results and reports it might be useful to see which exact members/properties of an object failed. What do you think?