Bug 169741

Created attachment 304625 [details] Patch

<rdar://problem/31085559>

Created attachment 304647 [details] Patch

Created attachment 304648 [details] Patch

Comment on attachment 304648 [details] Patch Changing the API should be covered by tests/test updates. Also, we need to understand how this might break web sites or not. Hopefully shims might do the trick here. View in context: https://bugs.webkit.org/attachment.cgi?id=304648&action=review > Source/WebCore/Modules/mediastream/RTCEnums.h:30 > +#include "RTCSdpType.h" I don't really like these headers being just include of other headers. This is handy for the code but may end up be inefficient when compiling. > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:247 > + Ref<RTCSessionDescription> description = RTCSessionDescription::create(init); auto here probably, or better a one liner. This is also inefficient. We are probably allocation a description that we will free just afterwards. If we are keeping a ref of description, we should move description. If not, we are allocating then freeing, this is not really good and we would be better either pass init or some sort of structure allocated on the stack. > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:274 > + m_backend->setRemoteDescription(description.get(), WTFMove(promise)); Ditto. > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:76 > + [JSBuiltin] Promise<void> setLocalDescription(RTCSessionDescriptionInit description); This is a change of behavior. Have you checked whether this is breaking web sites or not? There should be tests covering that change also.

Maybe the patch could be split in two, one about clean-up and one about IDL observable changes

Comment on attachment 304648 [details] Patch Attachment 304648 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/3339427 Number of test failures exceeded the failure limit.

Created attachment 304660 [details] Archive of layout-test-results from ews104 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 304648 [details] Patch Attachment 304648 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3339422 Number of test failures exceeded the failure limit.

Created attachment 304661 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 304648 [details] Patch Attachment 304648 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3343123 Number of test failures exceeded the failure limit.

Created attachment 304715 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

> > Source/WebCore/Modules/mediastream/RTCPeerConnection.idl:76 > > + [JSBuiltin] Promise<void> setLocalDescription(RTCSessionDescriptionInit description); > > This is a change of behavior. > Have you checked whether this is breaking web sites or not? > There should be tests covering that change also. I was wrong. setLocalDescription being JSBuiltin, the cast check is handled by the JS built-in itself

Comment on attachment 304648 [details] Patch r = me, provided a plan for session description handling

(In reply to comment #14) > Comment on attachment 304648 [details] > Patch > > r = me, provided a plan for session description handling Thanks. I'm going to redo this and separate RTCSdpType from the session description stuff, and moving the enums into one place.

Created attachment 304844 [details] Patch

Comment on attachment 304844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304844&action=review > Source/WebCore/Modules/mediastream/RTCIceConnectionState.h:29 > +#include "RTCEnums.h" Given binding generator requires having an RTCIceConnectionState.h file, why not defining its enum here and moving it to WebCore/platform? The alternative would be to hack the binding generator if we want this pattern, but I don't think it is worth it. Ditto for other files. > Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:214 > + transceiver.setDirection(static_cast<RTCRtpTransceiverDirection>(init.direction)); I don't think we want that static_cast. Can we find a way to do without it? If we really cannot, let's add a dedicated conversion routine. > Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:72 > + case RTCRtpTransceiverDirection::Inactive: return inactiveString(); Not following WebKit style. Probably redundant with code generated by binding generator. If so, can we find a way to reuse the generated code? > Source/WebCore/Modules/mediastream/RTCSessionDescription.h:36 > +#include "RTCEnums.h" If we were to define the RTCSdpType enum in RTCSdpType.h, we would only need to include RTCSdpType.h. This seems a better approach than using a global RTCEnums.h. > Source/WebCore/Modules/mediastream/RTCSessionDescription.h:57 > + explicit RTCSessionDescription(RTCSdpType, const String& sdp); No need for explicit. sdp should be made a String&& (and create as well). > Source/WebCore/Modules/mediastream/RTCSessionDescription.idl:41 > +}; Probably a matter of style, some prefer a one-liner enum when it can fit on one line, which is the case here.

Comment on attachment 304844 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=304844&action=review >> Source/WebCore/Modules/mediastream/RTCIceConnectionState.h:29 >> +#include "RTCEnums.h" > > Given binding generator requires having an RTCIceConnectionState.h file, why not defining its enum here and moving it to WebCore/platform? > The alternative would be to hack the binding generator if we want this pattern, but I don't think it is worth it. > > Ditto for other files. Ok, I will move all of the enumeration headers to platform. >> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:214 >> + transceiver.setDirection(static_cast<RTCRtpTransceiverDirection>(init.direction)); > > I don't think we want that static_cast. > Can we find a way to do without it? > If we really cannot, let's add a dedicated conversion routine. Removed. >> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:72 >> + case RTCRtpTransceiverDirection::Inactive: return inactiveString(); > > Not following WebKit style. > Probably redundant with code generated by binding generator. > If so, can we find a way to reuse the generated code? It is not redundant. There is no generation of enum to WTF strings. It's only used here so I kept it here.

Created attachment 304881 [details] Patch

Created attachment 304882 [details] Patch

> >> Source/WebCore/Modules/mediastream/RTCRtpTransceiver.cpp:72 > >> + case RTCRtpTransceiverDirection::Inactive: return inactiveString(); > > > > Not following WebKit style. > > Probably redundant with code generated by binding generator. > > If so, can we find a way to reuse the generated code? > > It is not redundant. There is no generation of enum to WTF strings. It's > only used here so I kept it here. Ah right, it is not used for binding code so something like toJS<IDLEnumeration<RTCSessionDescription::SdpType>> would not work. I see that both MediaEndpointSessionDescription and LibWebRTCEndpoint have about the same routine to convert the enum into a String/const char*. It might be good to have just one such routine, the other being a wrapper. Or add a FIXME.

Created attachment 304991 [details] Patch for landing

Comment on attachment 304991 [details] Patch for landing Rejecting attachment 304991 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 304991, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/3374582

Created attachment 304992 [details] Patch for landing

Comment on attachment 304992 [details] Patch for landing Clearing flags on attachment: 304992 Committed r214212: <http://trac.webkit.org/changeset/214212>