Bug 177193 - Switch PeerConnection to release logging
Summary: Switch PeerConnection to release logging
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-19 14:21 PDT by Eric Carlson
Modified: 2017-11-29 13:45 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch. (33.04 KB, patch)
2017-09-19 14:58 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Proposed patch. (31.91 KB, patch)
2017-09-19 21:49 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing. (31.35 KB, patch)
2017-09-20 10:35 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2017-09-19 14:21:45 PDT
Switch existing PeerConnection logging to release logging.
Comment 1 Radar WebKit Bug Importer 2017-09-19 14:22:29 PDT
<rdar://problem/34529014>
Comment 2 Eric Carlson 2017-09-19 14:58:29 PDT
Created attachment 321247 [details]
Proposed patch.
Comment 3 youenn fablet 2017-09-19 16:03:46 PDT
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.
Comment 4 Eric Carlson 2017-09-19 21:49:46 PDT
Created attachment 321288 [details]
Proposed patch.
Comment 5 Eric Carlson 2017-09-19 21:52:33 PDT
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.
Comment 6 youenn fablet 2017-09-20 09:54:18 PDT
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.
Comment 7 youenn fablet 2017-09-20 09:54:33 PDT
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.
Comment 8 Eric Carlson 2017-09-20 10:35:41 PDT
Created attachment 321330 [details]
Patch for landing.
Comment 9 WebKit Commit Bot 2017-09-20 10:55:31 PDT
Comment on attachment 321330 [details]
Patch for landing.

Clearing flags on attachment: 321330

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