Skip to content
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

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

Closed
foolip opened this issue Sep 15, 2016 · 15 comments
Closed
Assignees
Milestone

Comments

@foolip
Copy link
Member

foolip commented Sep 15, 2016

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 mwatson2 added this to the V1NonBlocking milestone Sep 15, 2016
@mwatson2
Copy link
Contributor

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
Copy link
Contributor

ddorwin commented Sep 15, 2016

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
Copy link
Member Author

foolip commented Sep 15, 2016

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
Copy link
Member Author

foolip commented Sep 15, 2016

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

@foolip
Copy link
Member Author

foolip commented Sep 15, 2016

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
Copy link
Contributor

ddorwin commented Sep 15, 2016

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
Copy link
Member Author

foolip commented Sep 15, 2016

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
Copy link
Contributor

@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
Copy link
Member Author

foolip commented Sep 15, 2016

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
Copy link
Contributor

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
Copy link
Member Author

foolip commented Sep 15, 2016

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

@foolip
Copy link
Member Author

foolip commented Sep 16, 2016

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
Copy link
Contributor

ddorwin commented Sep 16, 2016

@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
Copy link
Contributor

ddorwin commented Sep 16, 2016

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 added a commit to ddorwin/encrypted-media that referenced this issue Sep 16, 2016
…dictionary

Also changes some init dictionary attributes to be required and not have default values.
@ddorwin ddorwin self-assigned this Sep 16, 2016
@ddorwin
Copy link
Contributor

ddorwin commented Sep 16, 2016

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

ddorwin added a commit to ddorwin/web-platform-tests that referenced this issue Sep 20, 2016
ddorwin added a commit to web-platform-tests/wpt that referenced this issue Sep 20, 2016
ddorwin added a commit to ddorwin/encrypted-media that referenced this issue Sep 20, 2016
…alue

The prose was not updated with the IDL in 5499821.
ddorwin added a commit that referenced this issue Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants