RESOLVED FIXED 158536
WebRTC: The RTCTrackEventInit dictionary needs required members
https://bugs.webkit.org/show_bug.cgi?id=158536
Summary WebRTC: The RTCTrackEventInit dictionary needs required members
Adam Bergkvist
Reported Wednesday, June 8, 2016 8:20:05 PM UTC
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
Attachments
Patch (6.31 KB, patch)
2016-10-20 10:11 PDT, Nael Ouedraogo
no flags
Archive of layout-test-results from ews101 for mac-yosemite (1016.76 KB, application/zip)
2016-10-20 11:08 PDT, Build Bot
no flags
Patch (9.19 KB, patch)
2016-10-26 03:10 PDT, Nael Ouedraogo
no flags
Nael Ouedraogo
Comment 1 Thursday, October 20, 2016 5:50:20 PM UTC
(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
Comment 2 Thursday, October 20, 2016 5:51:05 PM UTC
[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
Comment 3 Thursday, October 20, 2016 5:59:29 PM UTC
(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
Comment 4 Thursday, October 20, 2016 6:03:09 PM UTC
(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
Comment 5 Thursday, October 20, 2016 6:07:59 PM UTC
Reopened to update RTCTrackEvent IDL.
Nael Ouedraogo
Comment 6 Thursday, October 20, 2016 6:11:01 PM UTC
Build Bot
Comment 7 Thursday, October 20, 2016 7:08:15 PM UTC
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
Comment 8 Thursday, October 20, 2016 7:08:19 PM UTC
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
Comment 9 Friday, October 21, 2016 10:46:27 AM UTC
(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 10 Friday, October 21, 2016 5:01:46 PM UTC
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
Comment 11 Monday, October 24, 2016 8:28:13 AM UTC
(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
Comment 12 Monday, October 24, 2016 3:37:05 PM UTC
(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
Comment 13 Wednesday, October 26, 2016 11:10:21 AM UTC
Nael Ouedraogo
Comment 14 Wednesday, October 26, 2016 11:13:47 AM UTC
> > 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 15 Wednesday, October 26, 2016 1:57:37 PM UTC
Comment on attachment 292912 [details] Patch Clearing flags on attachment: 292912 Committed r207895: <http://trac.webkit.org/changeset/207895>
WebKit Commit Bot
Comment 16 Wednesday, October 26, 2016 1:57:42 PM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.