[webkit-reviews] review granted: [Bug 186969] Remove static initializers more : [Attachment 343492] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 186969: Remove static initializers more
https://bugs.webkit.org/show_bug.cgi?id=186969

Attachment 343492: Patch

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




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 343492
  --> https://bugs.webkit.org/attachment.cgi?id=343492
Patch

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

Yes, exciting patch!

I'm not sure about the ResourceUsageData change... the code was surely easier
to read the way it was before. Does your change make it a trivially
destructible type? If so, then I suppose it is good, to avoid static
initializers and destructors.

> Source/WebCore/ChangeLog:8
> +	   This patch removes static initializers more. They typically exists
in GTK port.

Yes, static initializers are very tempting, but we should really avoid them.

We just added one in RenderThemeGtk, in fact, that should really have used
NeverDestroyed instead. (CC Carlos Eduardo ;)

> Source/WebCore/ChangeLog:30
> +	   * platform/network/soup/SoupNetworkSession.cpp:

I swear I did this just yesterday, as part of an effort to reduce asan
warnings, then gave up and threw away the work when I noticed the next issue
(PasteboardHelper) and assumed there would be an unending number of them.
Anyway, just an interesting coincidence to note.

>
Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:4
0
> -const static CapabilityValueOrRange defaultVolumeCapability =
CapabilityValueOrRange(0.0, 1.0);
> +static CapabilityValueOrRange defaultVolumeCapability()
> +{
> +    return CapabilityValueOrRange(0.0, 1.0);
> +}

Of course the difference is that now a new CapabilityValueOrRange will be
constructed every time this function is called. I guess that's cheap?

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:58
>  static bool gIgnoreTLSErrors;
> -static CString gInitialAcceptLanguages;
> -static SoupNetworkProxySettings gProxySettings;
> +static CString& initialAcceptLanguages()
> +{
> +    static NeverDestroyed<CString> storage;
> +    return storage.get();
> +}
> +static SoupNetworkProxySettings proxySettings()
> +{
> +    static NeverDestroyed<SoupNetworkProxySettings> settings;
> +    return settings.get();
> +}
>  static GType gCustomProtocolRequestType;

How about some blank lines above and below the functions? And move
gCustomProtocolRequestType up beneath gIgnoreTLSErrors.

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:53
> +static WTF::Vector<unsigned>& listenerIds()
> +{
> +    static NeverDestroyed<WTF::Vector<unsigned>> ids;
> +    return ids.get();
> +}
> +static NotificationHandlersMap& notificationHandlers()
> +{
> +    static NeverDestroyed<NotificationHandlersMap> map;
> +    return map.get();
> +}
>  AccessibilityNotificationHandler* globalNotificationHandler = nullptr;

Again, I think it's easier to read when there are more blank lines!