Created attachment 304476 [details] Patch

<rdar://problem/31056344>

Created attachment 304480 [details] Patch

Comment on attachment 304480 [details] Patch LGTM. I am not sure I love the one enum/one IDL/one header thing though since these make a lot of very small files. View in context: https://bugs.webkit.org/attachment.cgi?id=304480&action=review > Source/WebCore/Modules/mediastream/RTCEnums.h:29 > +#include "RTCRtpTransceiverDirection.h" Do we really need this include? > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:58 > +struct RTCRtpTransceiverInit { I guess it will move to its own header at some point? > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:62 > + JSBuiltinConstructor Matter of style I guess, ',' allows to not touch the line should we add a new keyword after it. > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:136 > + // Promise<void> addIceCandidate(RTCIceCandidate candidate, VoidCallback successCallback, RTCPeerConnectionErrorCallback errorCallback); I am not sure we need these, a comment stating that overloaded legacy operations are handled in RTCPeerConnection.js seems good enough. > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:168 > + Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null); selector will go away. I would probably just add a big FIXME 169644, but if you want to be precise, you should also add a fix here.

(In reply to comment #4) > Comment on attachment 304480 [details] > Patch > > LGTM. > I am not sure I love the one enum/one IDL/one header thing though since > these make a lot of very small files. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=304480&action=review > > > Source/WebCore/Modules/mediastream/RTCEnums.h:29 > > +#include "RTCRtpTransceiverDirection.h" > > Do we really need this include? I was meaning: do we really need RTCEnums.h? > > Source/WebCore/Modules/mediastream/RTCPeerConnection.h:58 > > +struct RTCRtpTransceiverInit { > > I guess it will move to its own header at some point? > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:62 > > + JSBuiltinConstructor > > Matter of style I guess, ',' allows to not touch the line should we add a > new keyword after it. > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:136 > > + // Promise<void> addIceCandidate(RTCIceCandidate candidate, VoidCallback successCallback, RTCPeerConnectionErrorCallback errorCallback); > > I am not sure we need these, a comment stating that overloaded legacy > operations are handled in RTCPeerConnection.js seems good enough. > > > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:168 > > + Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null); > > selector will go away. > I would probably just add a big FIXME 169644, but if you want to be precise, > you should also add a fix here.

Comment on attachment 304480 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304480&action=review >>> Source/WebCore/Modules/mediastream/RTCEnums.h:29 >>> +#include "RTCRtpTransceiverDirection.h" >> >> Do we really need this include? > > I was meaning: do we really need RTCEnums.h? More will be added here in subsequent patches. I think it's nicer to not have to include every enum's header file in implementations. Another possible way of doing this would be to move the enums to PeerConnectionStates, and then just include that header file for each of the individual enum header files. >>> Source/WebCore/Modules/mediastream/RTCPeerConnection.h:58 >>> +struct RTCRtpTransceiverInit { >> >> I guess it will move to its own header at some point? > > Yes, though I don't think any other file uses it. >> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:62 >> + JSBuiltinConstructor > > Matter of style I guess, ',' allows to not touch the line should we add a new keyword after it. Ok. >> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:136 >> + // Promise<void> addIceCandidate(RTCIceCandidate candidate, VoidCallback successCallback, RTCPeerConnectionErrorCallback errorCallback); > > I am not sure we need these, a comment stating that overloaded legacy operations are handled in RTCPeerConnection.js seems good enough. Done. >> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:168 >> + Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null); > > selector will go away. > I would probably just add a big FIXME 169644, but if you want to be precise, you should also add a fix here. This is a more recent development than what's in the spec?

(In reply to comment #4) > Comment on attachment 304480 [details] > Patch > > LGTM. > I am not sure I love the one enum/one IDL/one header thing though since > these make a lot of very small files. For enums that are used in other IDLs, this is what is currently required by the code generator.

> >> Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:168 > >> + Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null); > > > > selector will go away. > > I would probably just add a big FIXME 169644, but if you want to be precise, you should also add a fix here. > > This is a more recent development than what's in the spec? Ah, this might not have landed yet.

Committed r213992: <http://trac.webkit.org/changeset/213992>