Bug 169664 - Flatten RTC enum naming
Summary: Flatten RTC enum naming
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on:
Blocks: 169662
  Show dependency treegraph
 
Reported: 2017-03-15 00:17 PDT by Jon Lee
Modified: 2017-03-15 23:10 PDT (History)
2 users (show)

See Also:


Attachments
Patch (92.53 KB, patch)
2017-03-15 00:44 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (91.33 KB, patch)
2017-03-15 13:36 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Patch (92.89 KB, patch)
2017-03-15 15:19 PDT, Jon Lee
youennf: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2017-03-15 00:17:07 PDT
RTC enums are often used as subtypes of classes. Unify them to use the same naming throughout.
Comment 1 Jon Lee 2017-03-15 00:44:53 PDT
Created attachment 304486 [details]
Patch
Comment 2 Jon Lee 2017-03-15 00:45:15 PDT
Patch is built on top of 169660.
Comment 3 youenn fablet 2017-03-15 08:56:48 PDT
Comment on attachment 304486 [details]
Patch

LGTM!
Some comments below.
Will r+ once rebased.

View in context: https://bugs.webkit.org/attachment.cgi?id=304486&action=review

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:333
> +    if (m_peerConnection.signalingState() == RTCSignalingState::Closed)

Nice!

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:821
> +        case RTCIceTransportState::New: ++newCount; break;

This does not match WebKit style.

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:865
> +    Vector<RTCIceTransportState> transportStates;

I know this is untouched code.
Would you be able to use reserveInitialCapacity/uncheckedAppend

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:870
> +    m_peerConnection.updateIceConnectionState(static_cast<RTCIceConnectionState>(derivedState));

This looks bad, deriveAggregatedIceConnectionState should really be changed to return RTCIceConnectionState.

> Source/WebCore/Modules/mediastream/RTCConfiguration.idl:29
> +    ImplementedAs=RTCIceTransportPolicy

Out of curiosity why do you need to add ImplementedAs? Is it because otherwise it would be named RTCConfiguration::RTCIceTransportPolicy?

> Source/WebCore/Modules/mediastream/RTCConfiguration.idl:39
> +] enum RTCBundlePolicy {

Given the direction, I guess we should put RTCBundlePolicy and RTCIceTransportPolicy enum class definitions in RTCConfiguration.h
It is not very clear why this enum does not have its own IDL file and others will. I guess it is ok anyway.

> Source/WebCore/Modules/mediastream/RTCIceConnectionState.h:28
> +#include "PeerConnectionStates.h"

That seems a bit strange, could we not move connection state enum here and include this file in PeerConnectionStates.h?
Ditto for the other files.
Comment 4 Jon Lee 2017-03-15 11:31:33 PDT
Comment on attachment 304486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304486&action=review

>> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:821
>> +        case RTCIceTransportState::New: ++newCount; break;
> 
> This does not match WebKit style.

I can split these out into separate lines.

>> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:865
>> +    Vector<RTCIceTransportState> transportStates;
> 
> I know this is untouched code.
> Would you be able to use reserveInitialCapacity/uncheckedAppend

I prefer not doing fly-by fixes in these kinds of patches; it'll be confusing during blaming.

>> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:870
>> +    m_peerConnection.updateIceConnectionState(static_cast<RTCIceConnectionState>(derivedState));
> 
> This looks bad, deriveAggregatedIceConnectionState should really be changed to return RTCIceConnectionState.

Ditto.

>> Source/WebCore/Modules/mediastream/RTCConfiguration.idl:29
>> +    ImplementedAs=RTCIceTransportPolicy
> 
> Out of curiosity why do you need to add ImplementedAs? Is it because otherwise it would be named RTCConfiguration::RTCIceTransportPolicy?

Then it is generated as RTCIceTransportPolicy rather than RTCConfiguration::IceTransportPolicy (the script removes common prefixes).

>> Source/WebCore/Modules/mediastream/RTCConfiguration.idl:39
>> +] enum RTCBundlePolicy {
> 
> Given the direction, I guess we should put RTCBundlePolicy and RTCIceTransportPolicy enum class definitions in RTCConfiguration.h
> It is not very clear why this enum does not have its own IDL file and others will. I guess it is ok anyway.

I include enums in the same file as the main IDL definition if they are not used elsewhere in the spec. If they are used in multiple places, then the enum has to be declared separately.

>> Source/WebCore/Modules/mediastream/RTCIceConnectionState.h:28
>> +#include "PeerConnectionStates.h"
> 
> That seems a bit strange, could we not move connection state enum here and include this file in PeerConnectionStates.h?
> Ditto for the other files.

This would be a layering violation, no?
Comment 5 Jon Lee 2017-03-15 13:36:07 PDT
Created attachment 304538 [details]
Patch
Comment 6 Radar WebKit Bug Importer 2017-03-15 13:37:12 PDT
<rdar://problem/31072224>
Comment 7 Jon Lee 2017-03-15 15:19:06 PDT
Created attachment 304558 [details]
Patch
Comment 8 Jon Lee 2017-03-15 15:44:14 PDT
(In reply to comment #3)
> 
> 
> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:865
> > +    Vector<RTCIceTransportState> transportStates;
> 
> I know this is untouched code.
> Would you be able to use reserveInitialCapacity/uncheckedAppend
> 
> > Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:870
> > +    m_peerConnection.updateIceConnectionState(static_cast<RTCIceConnectionState>(derivedState));
> 
> This looks bad, deriveAggregatedIceConnectionState should really be changed
> to return RTCIceConnectionState.
> 

For these two comments, filed bug 169709.
Comment 9 youenn fablet 2017-03-15 15:45:01 PDT
> > I know this is untouched code.
> > Would you be able to use reserveInitialCapacity/uncheckedAppend
> 
> I prefer not doing fly-by fixes in these kinds of patches; it'll be
> confusing during blaming.

This kind of refactoring is usual, like doing Ref/RefPtr things.

> >> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:870
> >> +    m_peerConnection.updateIceConnectionState(static_cast<RTCIceConnectionState>(derivedState));
> > 
> > This looks bad, deriveAggregatedIceConnectionState should really be changed to return RTCIceConnectionState.
> 
> Ditto.

Put a FIXME then?
Comment 10 Jon Lee 2017-03-15 23:10:57 PDT
Committed r214030: <http://trac.webkit.org/changeset/214030>