Bug 228768 – Support WTF logging channels

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Support WTF logging channels

[Myles C. Maxfield]

Created attachment 434895 [details]
Patch

[Myles C. Maxfield]

Created attachment 434896 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 434896 [details]
Patch

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

r- to remove code duplicated between WTF, WebCore and WebKit.

> Source/WTF/wtf/Logging.h:57
> +WTF_EXPORT_PRIVATE String logLevelString();
> +bool isLogChannelEnabled(const String& name);
> +WTF_EXPORT_PRIVATE void setLogChannelToAccumulate(const String& name);
> +WTF_EXPORT_PRIVATE void clearAllLogChannelsToAccumulate();
> +WTF_EXPORT_PRIVATE void initializeLogChannelsIfNecessary(std::optional<String> = std::nullopt);

I think you should follow WebCore and put these in LogInitialization.h (there was a reason I split things into two headers, but I forget what it was).

I think you can also remove the WebCore and WebKit implementations of these functions, sharing all the code from WTF.

[Myles C. Maxfield]

Created attachment 435121 [details]
Patch

[Myles C. Maxfield]

Created attachment 435122 [details]
Patch

[Myles C. Maxfield]

Created attachment 435123 [details]
Patch

[Fujii Hironori]

You might want to update the documentation. https://github.com/WebKit/WebKit/blob/main/Introduction.md#logging-in-webkit

[Myles C. Maxfield]

(In reply to Fujii Hironori from comment #7)
> You might want to update the documentation.
> https://github.com/WebKit/WebKit/blob/main/Introduction.md#logging-in-webkit

Yeah, good idea. It looks like nothing that's currently written there is inaccurate with these latest patches, but I can add some more details which will make it more clear how to do things like enable/disable log channels.

[Myles C. Maxfield]

Also, I can add information about how to add a new log channel, which macro to use where, etc.

[Myles C. Maxfield]

https://bugs.webkit.org/show_bug.cgi?id=228899

[Fujii Hironori]

Comment on attachment 435123 [details]
Patch

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

> Source/WTF/wtf/MemoryPressureHandler.cpp:56
> +#endif

Do you still need to explicitly call WTFInitializeLogChannelStatesFromString for MemoryPressure log channel?
I think WTF::logChannels().initializeLogChannelsIfNecessary() does the initialization even for MemoryPressure log channel.

> Source/WTF/wtf/unix/LoggingUnix.cpp:38
> +        return makeString("-all"_s);

This seems unexpected behavior change. This disable all WTF log channels as default if WEBKIT_DEBUG is not set.

> Source/WTF/wtf/unix/LoggingUnix.cpp:46
> +    return makeString("NotYetImplemented,"_s, logEnv);

This seems useless. WTF module doesn't have "NotYetImplemented" log channel.

[Myles C. Maxfield]

Comment on attachment 435123 [details]
Patch

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

>> Source/WTF/wtf/MemoryPressureHandler.cpp:56
>> +#endif
> 
> Do you still need to explicitly call WTFInitializeLogChannelStatesFromString for MemoryPressure log channel?
> I think WTF::logChannels().initializeLogChannelsIfNecessary() does the initialization even for MemoryPressure log channel.

Right! Good catch.

>> Source/WTF/wtf/unix/LoggingUnix.cpp:38
>> +        return makeString("-all"_s);
> 
> This seems unexpected behavior change. This disable all WTF log channels as default if WEBKIT_DEBUG is not set.

Right, yes. I copied this from WebCore, but you're right that I shouldn't be changing this behavior.

>> Source/WTF/wtf/unix/LoggingUnix.cpp:46
>> +    return makeString("NotYetImplemented,"_s, logEnv);
> 
> This seems useless. WTF module doesn't have "NotYetImplemented" log channel.

Right.

[Myles C. Maxfield]

Created attachment 435163 [details]
Patch

[Fujii Hironori]

Comment on attachment 435163 [details]
Patch

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

LGTM

> Source/WTF/wtf/unix/LoggingUnix.cpp:41
> +    // To disable logging notImplemented set the DISABLE_NI_WARNING environment variable to 1.

Remove this comment.

[Myles C. Maxfield]

Committed r280795 (240373@main): <https://commits.webkit.org/240373@main>

[Radar WebKit Bug Importer]

<rdar://problem/81713600>

[Fujii Hironori]

(In reply to Myles C. Maxfield from comment #15)
> Committed r280795 (240373@main): <https://commits.webkit.org/240373@main>

This change caused build failures for Debug builds of Win, GTK and WPE ports.

  Bug 228938 – [GTK][WPE] REGRESSION(r280795): MemoryPressureHandlerUnix.cpp:45:28: error: ‘LogMemoryPressure’ was not declared in this scope
  Bug 228937 – [WTF][Win] REGRESSION(r280795) error LNK2019: unresolved external symbol "class WTF::String __cdecl WTF::logLevelString(void)"

[Myles C. Maxfield]

(In reply to Fujii Hironori from comment #17)
> (In reply to Myles C. Maxfield from comment #15)
> > Committed r280795 (240373@main): <https://commits.webkit.org/240373@main>
> 
> This change caused build failures for Debug builds of Win, GTK and WPE ports.
> 
>   Bug 228938 – [GTK][WPE] REGRESSION(r280795):
> MemoryPressureHandlerUnix.cpp:45:28: error: ‘LogMemoryPressure’ was not
> declared in this scope
>   Bug 228937 – [WTF][Win] REGRESSION(r280795) error LNK2019: unresolved
> external symbol "class WTF::String __cdecl WTF::logLevelString(void)"

Oh, no :(

EWS was green though :(

Thanks for fixing it!!!