w3c / encrypted-media

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

MediaKeyMessageEvent's message needs to be nullable or required in init dict #329

Closed foolip closed 8 years ago

foolip commented 8 years ago

https://w3c.github.io/encrypted-media/#mediakeymessageevent

Per the current spec, new MediaKeyMessageEvent('type') would not throw an exception, because the init dict is not required. However, the result event's message attribute cannot sensibly be anything other than null, and yet it's not nullable in the IDL.

The typical pattern for event interfaces where only scripts could create instances with a certain member being null is to make that impossible, as such:

[SecureContext,
 Constructor(DOMString type, MediaKeyMessageEventInit eventInitDict)]
interface MediaKeyMessageEvent : Event {
    readonly attribute MediaKeyMessageType messageType;
    readonly attribute ArrayBuffer         message;
};

[SecureContext]
dictionary MediaKeyMessageEventInit : EventInit {
    MediaKeyMessageType messageType = "license-request";
    required ArrayBuffer message;
};

If there is a concern for breaking existing content, the only other option is unfortunately to make message nullable.

mwatson2 commented 8 years ago

I think the eventInitDict parameter should be made mandatory in the constructor as well as making the message attribute required in the MediaKeyMessageEventInit dictionary.

The specification as it stands is underspecified because the contents of the message attribute is not defined for the case where the eventInitDict is not provided. So I don't think there is anything to break by making the change above (or rather, in the unlikely event anyone is using new MediaKeyMessageEvent( 'type' ) today, the behavior is not fully specified, and so could change under them anyway.)

ddorwin commented 8 years ago

This seems like a good change, and I doubt this will break any real applications.

Do we need to make MediaEncryptedEventInit non-optional for the MediaEncryptedEvent constructor too?

foolip commented 8 years ago

Yes, if the dictionary is still optional you'd still end up with the nullable message on the event interface itself, it has to be impossible to create an event without an ArrayBuffer.

foolip commented 8 years ago

(That was in reply to @ddorwin about making the init dict non-optional.)

foolip commented 8 years ago

Sorry again, I didn't see that @ddorwin was talking about a different event. For MediaEncryptedEvent, if you made initDict non-nullable on the interface and required in the init dict, that'd match what I was proposing for MediaKeyMessageEvent.

ddorwin commented 8 years ago

initData should be nullable because the Initialization Data Encountered algorithms may use null (i.e for cross-origin content). Can/should we still require the dict?

foolip commented 8 years ago

Oh, if the UA can create instances where initData is null, then trying to make the dict non-optional and its member required leads to a funny place. I learned today that in WebIDL passing null as a dictionary member is treated like missing, which for a required member is then a TypeError. So that would make it impossible for the script to create an instance where initData is null :(

mwatson2 commented 8 years ago

@foolip Is that true when the dictionary member has a default value ? In this case the default value is null so doesn't it just get the default whether it is explicitly included with a null value in the parameter or omitted ?

i.e.

new MediaEncryptedEvent( "encrypted", { initDataType: "cenc" } );

and

new MediaEncryptedEvent( "encrypted", { initDataType: "cenc", initData: null } );

are the same and result in a MediaEncryptedEvent dictionary as follows:

{ initDataType: "cenc", initData: null }
foolip commented 8 years ago

The bit that I learned today is in https://heycam.github.io/webidl/#es-dictionary step 1.5.1.2. There null is treated as a missing argument, so the two examples you gave would be treated identical, in fact any spec prose that comes "after" that conversion couldn't distinguish them at all.

mwatson2 commented 8 years ago

Ok. So that seems fine in our case (for MediaEncryptedEvent): we make the constructor parameter mandatory and leave the defaults in the initialization dictionary.

foolip commented 8 years ago

I think that MediaEncryptedEvent works IDL-wise as currently spec'd, but maybe I'm misunderstanding again :)

foolip commented 8 years ago

A lot of the things I wrote on this issue are wrong because I misunderstood https://heycam.github.io/webidl/#es-dictionary step 1.5.1.2. That actually checks the type of the thing to convert to a dictionary, not the type of the member. So { foo: null } is not treated as if foo is missing, and doesn't necessarily behave the same.

The initial suggestion about changes to MediaKeyMessageEvent still stands, though.

ddorwin commented 8 years ago

@foolip provided (offline) a useful guideline for the init dictionary: "In short, if it'd work to pass {} instead of omitting the argument, then it must be optional." That is the case if no members of the init dictionary are required.

I believe we could remove the default value ("") for initDataType and make it required. That would mean that {} would never work.

Assuming the above is possible, we could change the IDL to:

[Constructor(DOMString type, MediaEncryptedEventInit eventInitDict)]
...

dictionary MediaEncryptedEventInit : EventInit {
    required DOMString    initDataType;
    ArrayBuffer? initData = null;
};
ddorwin commented 8 years ago

Returning to MediaKeyMessageEventInit, should we make messageType required? I'm not sure there's a reason we gave it a default value other than other init dictionary members had default values, and it would be better if any app creating the event was explicit about the type of the message rather than potentially getting the wrong value.

Thus, we'd have:

[SecureContext]
dictionary MediaKeyMessageEventInit : EventInit {
    required MediaKeyMessageType messageType;
    required ArrayBuffer message;
};
ddorwin commented 8 years ago

I created PR #330 for reference (and eventually to be the agreed-upon fix).