[webkit-reviews] review denied: [Bug 237183] [Cocoa] Allow logging to be configured by NSDefaults (without regressing launch time) : [Attachment 453339] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jer Noble <jer.noble at apple.com>'s
request for review:
Bug 237183: [Cocoa] Allow logging to be configured by NSDefaults (without
regressing launch time)
https://bugs.webkit.org/show_bug.cgi?id=237183

Attachment 453339: Patch

https://bugs.webkit.org/attachment.cgi?id=453339&action=review




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 453339
  --> https://bugs.webkit.org/attachment.cgi?id=453339
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIProcessLogInitializationCocoa.mm:53
> +String wtfLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = [[NSUserDefaults
standardUserDefaults] stringForKey:@"WTFLogging"];
> +    return logString;
> +}
> +
> +String webCoreLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = [[NSUserDefaults
standardUserDefaults] stringForKey:@"WebCoreLogging"];
> +    return logString;
> +}
> +
> +String webKitLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = [[NSUserDefaults
standardUserDefaults] stringForKey:@"WebKit2Logging"];
> +    return logString;
> +}

We should not use NeverDestroyed here. That makes these non-thread-safe.
There’s a reason we don’t do that in the WTF versions of these functions.

If there’s a performance problem with calling NSUserDefaults multiple times,
and maybe that’s the whole point of the patch, we can cache the results, but
we’d need a thread-safe way to do it, which would involve call_once and not
using String for the cross-thread cached result. For example, we could use a
NeverDestroyed<RetainPtr<NSString>> if we initialize it in a thread-safe
manner, and create a new WTF::String each time these are called.

Unless I am missing something and these are always called on only a single
thread? If so we should use the version of NeverDestroyed that includes that
check perhaps?

> Source/WebKit/UIProcess/UIProcessLogInitialization.cpp:67
> +#if !PLATFORM(COCOA)
> +String wtfLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = WTF::logLevelString();
> +    return logString;
> +}
> +
> +String webCoreLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = WebCore::logLevelString();
> +    return logString;
> +}
> +
> +String webKitLogLevelString()
> +{
> +    static NeverDestroyed<String> logString = WebKit::logLevelString();
> +    return logString;
> +}
> +#endif

Ditto.