Bug 177193 – Switch PeerConnection to release logging

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[Eric Carlson]

Switch existing PeerConnection logging to release logging.

[Radar WebKit Bug Importer]

<rdar://problem/34529014>

[Eric Carlson]

Created attachment 321247 [details]
Proposed patch.

[youenn fablet]

Comment on attachment 321247 [details]
Proposed patch.

Looks good to me in general.
Some details below.

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

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:83
> +    ALWAYS_LOG(LOGIDENTIFIER, "Create offer failed:", exception.message());

Probably fine to remove the \n as you did since exception.message might be small and not multiline like the sdp.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:363
> +    ALWAYS_LOG(LOGIDENTIFIER, "Gathered ice candidate:\n", sdp);

sdp here is small so it may be fine as a one liner.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:76
> +    PeerConnectionBackend(RTCPeerConnection&);

Add explicit

> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:62
> +using namespace PAL;

Do we really need it?

> Source/WebCore/platform/mediastream/RTCIceConnectionState.h:50
> +    static String toString(const WebCore::RTCIceConnectionState state)

JSRTCIceConnectionState.cpp/.h have the same function called convertEnumerationToString().
We can probably try piggybacking on it.

> Source/WebCore/platform/mediastream/RTCIceConnectionState.h:68
> +        ASSERT_NOT_REACHED();

I am not sure we need it anymore since we have compilation errors if not case-ing all enumeration values.

[Eric Carlson]

Created attachment 321288 [details]
Proposed patch.

[Eric Carlson]

Comment on attachment 321247 [details]
Proposed patch.

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

>> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:83
>> +    ALWAYS_LOG(LOGIDENTIFIER, "Create offer failed:", exception.message());
> 
> Probably fine to remove the \n as you did since exception.message might be small and not multiline like the sdp.

There isn't a \n in this one.

>> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:363
>> +    ALWAYS_LOG(LOGIDENTIFIER, "Gathered ice candidate:\n", sdp);
> 
> sdp here is small so it may be fine as a one liner.

Fixed.

>> Source/WebCore/Modules/mediastream/PeerConnectionBackend.h:76
>> +    PeerConnectionBackend(RTCPeerConnection&);
> 
> Add explicit

Fixed.

>> Source/WebCore/Modules/mediastream/RTCPeerConnection.cpp:62
>> +using namespace PAL;
> 
> Do we really need it?

Nope.

>> Source/WebCore/platform/mediastream/RTCIceConnectionState.h:50
>> +    static String toString(const WebCore::RTCIceConnectionState state)
> 
> JSRTCIceConnectionState.cpp/.h have the same function called convertEnumerationToString().
> We can probably try piggybacking on it.

Changed this and RTCPeerConnectionState to use convertEnumerationToString.

>> Source/WebCore/platform/mediastream/RTCIceConnectionState.h:68
>> +        ASSERT_NOT_REACHED();
> 
> I am not sure we need it anymore since we have compilation errors if not case-ing all enumeration values.

Unnecessary since it now just calls convertEnumerationToString.

[youenn fablet]

Comment on attachment 321288 [details]
Proposed patch.

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

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:223
> +    ALWAYS_LOG(LOGIDENTIFIER, "Set remote description succeeded");

It seems ALWAYS_LOG is taking LOGIDENTIFIER as first parameter in almost all of the code base right now.
Maybe ALWAYS_LOG should by default log the identifier without the need to pass LOGIDENTIFIER.
This would end-up with a shorter ALWAYS_LOG. Ditto for INFO_LOG.

> Source/WebCore/platform/mediastream/RTCIceGatheringState.h:48
> +    static String toString(const WebCore::RTCIceGatheringState state)

There is probably a convertEnumerationToString for this one too.

> Source/WebCore/platform/mediastream/RTCSignalingState.h:50
> +        switch (state) {

Ditto.

[youenn fablet]

Comment on attachment 321288 [details]
Proposed patch.

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

>> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:223
>> +    ALWAYS_LOG(LOGIDENTIFIER, "Set remote description succeeded");
> 
> It seems ALWAYS_LOG is taking LOGIDENTIFIER as first parameter in almost all of the code base right now.
> Maybe ALWAYS_LOG should by default log the identifier without the need to pass LOGIDENTIFIER.
> This would end-up with a shorter ALWAYS_LOG. Ditto for INFO_LOG.

It seems ALWAYS_LOG is taking LOGIDENTIFIER as first parameter in almost all of the code base right now.
Maybe ALWAYS_LOG should by default log the identifier without the need to pass LOGIDENTIFIER.
This would end-up with a shorter ALWAYS_LOG. Ditto for INFO_LOG.

>> Source/WebCore/platform/mediastream/RTCIceGatheringState.h:48
>> +    static String toString(const WebCore::RTCIceGatheringState state)
> 
> There is probably a convertEnumerationToString for this one too.

There is probably a convertEnumerationToString for this one too.

>> Source/WebCore/platform/mediastream/RTCSignalingState.h:50
>> +        switch (state) {
> 
> Ditto.

Ditto.

[Eric Carlson]

Created attachment 321330 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 321330 [details]
Patch for landing.

Clearing flags on attachment: 321330

Committed r222271: <http://trac.webkit.org/changeset/222271>