w3c / encrypted-media

Encrypted Media Extensions
https://w3c.github.io/encrypted-media/
Other
180 stars 80 forks source link

sessionTypes: sequence of DOMString vs sequence of MediaKeySessionType #447

Closed xhwang-chromium closed 1 year ago

xhwang-chromium commented 5 years ago

This is about the sessionTypes in MediaKeySystemConfiguration in the EME spec. For historically backgrounds, see issue #260 and #47.

The spec defines sessionTypes as:

sessionTypes of type sequence \<DOMString> A list of MediaKeySessionTypes that must be supported. All values must be supported.

This is causing an issue in 3.1.1.2 Get Supported Configuration and Consent, step 13 and 13.1

13 For each value in session types: 13.1 Let session type be the value.

Here the session type is still a DOMString, but is passed to Is persistent session type? algorithm, which expects a MediaKeySessionType value as input. So it seems we are missing a DOMString to MediaKeySessionType conversion somewhere.

Using a DOMString has another issue that it makes it much harder to catch typos. For example, if we get NotSupportedError for sessionTypes: ['persistent-licence'], the author of the JS player may think that persistent license is not supported, but actually it's just a typo in the "licence".

With that, should sessionTypes be defined as sequence\<MediaKeySessionType>? That'll solve both issues above. For the typo example, TypeError will be returned immediately.

One argument of using the DOMString is that it allows nice forward compatibility. See see issue #260 and #47. But at least we need to fix the missing type conversion in the spec.

domenic commented 5 years ago

The difference between TypeError and NotSupportedError is not material here. You can use TypeError plus DOMString if you like.

Personally I think the type of the error is not very important at all, and what matters is the message (which is implementation-defined). For example if the message said

Type 'persistent-licence' is not supported. The supported types are: 'persistent-license', 'x', 'y', 'z'.

then that's very clear and helpful.

domenic commented 5 years ago

As for conversions, I think specs generally convert between strings and enums implicitly; I don't think there's a need to "define a conversion", since enums are just strings. It's true though you need to have "Is persistent session type?" return true or false as appropriate for other string values, i.e. you'd need to add a default case.

In other words, the bug is not a type mismatch, the bug is a non-exhaustive switch statement.

xhwang-chromium commented 5 years ago

ddorwin@ just reminded me of the major difference between NotSupportedError and TypeError in this case, which is unique to EME. In one requestMediaKeySystemAccess() call, we can pass in a sequence of MediaKeySystemConfiguration, and the UA will go through each of them. If a MediaKeySystemConfiguration is not supported, e.g. having a unrecognized (e.g. newly added) MediaKeySessionType, it'll be skipped. So that the UA can look at the next config. With sequence, the TypeError will happen at the IDL binding layer right after requestMediaKeySystemAccess() is called, and the UA doesn't get a chance to look at any of the configs passed in. Sorry I missed that part in the original bug. With that I think a sequence of DOMString makes more sense.

The only case an exception is thrown after starting to evaluate each configs is at the end, when no configs are supported. The current spec says to throw an NotSupportedError, which makes more sense than a TypeError. So all of these seem good to me now.

Given that "specs generally convert between strings and enums implicitly", I agree with domenic that we don't need to define a conversion, but need to be exhaustive in the switch statement since we are dealing with strings.

ddorwin commented 5 years ago

Technically, no exception is thrown, requestMediaKeySystemAccess() just "Reject[s] [the] promise with a NotSupportedError."