WebRTC: The RTCTrackEventInit dictionary needs required members

[Adam Bergkvist]

An attribute marked with [InitializedByEventConstructor] seem to be nullable and non-required [1] by default. RTCTrackEventInit [2] needs non-nullable and required dictionary members.

[1] http://heycam.github.io/webidl/#required-dictionary-member
[2] https://w3c.github.io/webrtc-pc/archives/20160513/webrtc.html#idl-def-rtctrackeventinit

[Nael Ouedraogo]

(In reply to comment #0)
> An attribute marked with [InitializedByEventConstructor] seem to be nullable
> and non-required [1] by default. 

It is fixed now so we can update RTCTrackEvent IDL.

[Chris Dumez]

[InitializedByEventConstructor] is no longer supported. You should use a dictionary, as in the specification. Also, as far as I know, we support required dictionary members already.

Marking as Invalid, feel free to reopen if you find issues remain.

[Nael Ouedraogo]

(In reply to comment #2)
> [InitializedByEventConstructor] is no longer supported. You should use a
> dictionary, as in the specification. Also, as far as I know, we support
> required dictionary members already.
> 
> Marking as Invalid, feel free to reopen if you find issues remain.
Ok, I modified the bug title to update RTCTrackEvent IDL as described in comment #0. Is it better to file a new bug?

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > [InitializedByEventConstructor] is no longer supported. You should use a
> > dictionary, as in the specification. Also, as far as I know, we support
> > required dictionary members already.
> > 
> > Marking as Invalid, feel free to reopen if you find issues remain.
> Ok, I modified the bug title to update RTCTrackEvent IDL as described in
> comment #0. Is it better to file a new bug?

I do not mind either way.

[Nael Ouedraogo]

Reopened to update RTCTrackEvent IDL.

[Nael Ouedraogo]

Created attachment 292218 [details]
Patch

[Build Bot]

Comment on attachment 292218 [details]
Patch

Attachment 292218 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2332277

New failing tests:
compositing/iframes/page-cache-layer-tree.html

[Build Bot]

Created attachment 292225 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Nael Ouedraogo]

(In reply to comment #8)
> Created attachment 292225 [details]
> Archive of layout-test-results from ews101 for mac-yosemite
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-ews.
> Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

Failure observed in compositing/iframes/page-cache-layer-tree.html seems not related to the patch. 
Same test on mac-elcapitan successfully passed locally.

[Chris Dumez]

Comment on attachment 292218 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=292218&action=review

r=me with comments

> Source/WebCore/Modules/mediastream/RTCTrackEvent.idl:37
> +    readonly attribute sequence<MediaStream> streams;

Looks like this should be a FrozenArray while you're at it (We support it).

> LayoutTests/fast/mediastream/RTCTrackEvent-constructor.html:55
>                  shouldThrow("new RTCTrackEvent('eventType', { receiver: null, track: track, transceiver: transceiver})");

Could you please update the test so that it only passes if we throw a TypeError (and not other exception types).
e.g.

shouldThrowErrorName("new RTCTrackEvent('eventType', { receiver: null, track: track, transceiver: transceiver})", "TypeError");

[Adam Bergkvist]

(In reply to comment #10)
> > Source/WebCore/Modules/mediastream/RTCTrackEvent.idl:37
> > +    readonly attribute sequence<MediaStream> streams;
> 
> Looks like this should be a FrozenArray while you're at it (We support it).

Great. That would match the spec.

https://w3c.github.io/webrtc-pc/archives/20160913/webrtc.html#rtctrackevent

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > > Source/WebCore/Modules/mediastream/RTCTrackEvent.idl:37
> > > +    readonly attribute sequence<MediaStream> streams;
> > 
> > Looks like this should be a FrozenArray while you're at it (We support it).
> 
> Great. That would match the spec.
> 
> https://w3c.github.io/webrtc-pc/archives/20160913/webrtc.html#rtctrackevent

This is why I suggested it :)

[Nael Ouedraogo]

Created attachment 292912 [details]
Patch

[Nael Ouedraogo]

> > Source/WebCore/Modules/mediastream/RTCTrackEvent.idl:37
> > +    readonly attribute sequence<MediaStream> streams;
> 
> Looks like this should be a FrozenArray while you're at it (We support it).
Ok done in uploaded patch, I was not sure that it was supported.

> > LayoutTests/fast/mediastream/RTCTrackEvent-constructor.html:55
> >                  shouldThrow("new RTCTrackEvent('eventType', { receiver: null, track: track, transceiver: transceiver})");
> 
> Could you please update the test so that it only passes if we throw a
> TypeError (and not other exception types).
> e.g.
> 
> shouldThrowErrorName("new RTCTrackEvent('eventType', { receiver: null,
> track: track, transceiver: transceiver})", "TypeError");
Done in uploaded patch.

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 292912 [details]
Patch

Clearing flags on attachment: 292912

Committed r207895: <http://trac.webkit.org/changeset/207895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.