Bug 169741 – Clean up RTCSdpType, RTC enums and headers

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Jon Lee]

Clean up RTCSdpType and switch to RTCSessionDescriptionInit

[Jon Lee]

Created attachment 304625 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/31085559>

[Jon Lee]

Created attachment 304647 [details]
Patch

[Jon Lee]

Created attachment 304648 [details]
Patch

[youenn fablet]

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.

[youenn fablet]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[youenn fablet]

> > 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

[youenn fablet]

Comment on attachment 304648 [details]
Patch

r = me, provided a plan for session description handling

[Jon Lee]

(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.

[Jon Lee]

Created attachment 304844 [details]
Patch

[youenn fablet]

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.

[Jon Lee]

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.

[Jon Lee]

Created attachment 304881 [details]
Patch

[Jon Lee]

Created attachment 304882 [details]
Patch

[youenn fablet]

> >> 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.

[Jon Lee]

Created attachment 304991 [details]
Patch for landing

[WebKit Commit Bot]

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

[Jon Lee]

Created attachment 304992 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 304992 [details]
Patch for landing

Clearing flags on attachment: 304992

Committed r214212: <http://trac.webkit.org/changeset/214212>