Bug 169741

Summary: Clean up RTCSdpType, RTC enums and headers
Product: WebKit Reporter: Jon Lee <jonlee>
Component: WebKit Misc.Assignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, rniwa, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 169662    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews101 for mac-elcapitan
none
Patch
none
Patch
none
Patch
youennf: review+
Patch for landing
commit-queue: commit-queue-
Patch for landing none

Description Jon Lee 2017-03-16 02:31:47 PDT
Clean up RTCSdpType and switch to RTCSessionDescriptionInit
Comment 1 Jon Lee 2017-03-16 02:40:43 PDT
Created attachment 304625 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-03-16 02:41:35 PDT
<rdar://problem/31085559>
Comment 3 Jon Lee 2017-03-16 09:51:05 PDT
Created attachment 304647 [details]
Patch
Comment 4 Jon Lee 2017-03-16 09:56:12 PDT
Created attachment 304648 [details]
Patch
Comment 5 youenn fablet 2017-03-16 10:09:31 PDT
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.
Comment 6 youenn fablet 2017-03-16 10:16:10 PDT
Maybe the patch could be split in two, one about clean-up and one about IDL observable changes
Comment 7 Build Bot 2017-03-16 11:47:00 PDT
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.
Comment 8 Build Bot 2017-03-16 11:47:04 PDT
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 9 Build Bot 2017-03-16 11:48:40 PDT
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.
Comment 10 Build Bot 2017-03-16 11:48:43 PDT
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 11 Build Bot 2017-03-16 16:17:20 PDT
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.
Comment 12 Build Bot 2017-03-16 16:17:25 PDT
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
Comment 13 youenn fablet 2017-03-17 08:23:57 PDT
> > 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 14 youenn fablet 2017-03-17 08:24:57 PDT
Comment on attachment 304648 [details]
Patch

r = me, provided a plan for session description handling
Comment 15 Jon Lee 2017-03-17 10:54:08 PDT
(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.
Comment 16 Jon Lee 2017-03-17 18:59:33 PDT
Created attachment 304844 [details]
Patch
Comment 17 youenn fablet 2017-03-17 21:43:35 PDT
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 18 Jon Lee 2017-03-18 14:57:23 PDT
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.
Comment 19 Jon Lee 2017-03-18 15:16:30 PDT
Created attachment 304881 [details]
Patch
Comment 20 Jon Lee 2017-03-18 15:25:07 PDT
Created attachment 304882 [details]
Patch
Comment 21 youenn fablet 2017-03-20 21:09:07 PDT
> >> 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.
Comment 22 Jon Lee 2017-03-20 23:59:46 PDT
Created attachment 304991 [details]
Patch for landing
Comment 23 WebKit Commit Bot 2017-03-21 00:01:23 PDT
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
Comment 24 Jon Lee 2017-03-21 00:07:44 PDT
Created attachment 304992 [details]
Patch for landing
Comment 25 WebKit Commit Bot 2017-03-21 00:50:08 PDT
Comment on attachment 304992 [details]
Patch for landing

Clearing flags on attachment: 304992

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