[webkit-reviews] review granted: [Bug 130893] Replace DEPRECATED_DEFINE_STATIC_LOCAL by static NeverDestroyed<T> in loader : [Attachment 228058] Patch (with win build fix)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 130893: Replace DEPRECATED_DEFINE_STATIC_LOCAL by static NeverDestroyed<T>
in loader
https://bugs.webkit.org/show_bug.cgi?id=130893

Attachment 228058: Patch (with win build fix)
https://bugs.webkit.org/attachment.cgi?id=228058&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=228058&action=review


> Source/WebCore/loader/cache/CachedRawResource.cpp:202
>  static bool shouldIgnoreHeaderForCacheReuse(AtomicString headerName)

I think const AtomicString& would be better for this argument.

> Source/WebCore/loader/cache/CachedRawResource.cpp:215
> +    static NeverDestroyed<HashSet<AtomicString>> m_headers;
> +    if (m_headers.get().isEmpty()) {
> +	   m_headers.get().add("Accept");
> +	   m_headers.get().add("Cache-Control");
> +	   m_headers.get().add("Origin");
> +	   m_headers.get().add("Pragma");
> +	   m_headers.get().add("Purpose");
> +	   m_headers.get().add("Referer");
> +	   m_headers.get().add("User-Agent");
>      }
> -    return m_headers.contains(headerName);
> +    return m_headers.get().contains(headerName);

This is a bad idiom. For one thing, we don’t want to call isEmpty every time;
this should be one-time initialization, not an empty check every time through.
For another, calling add multiple times results in an unrolled loop. We should
loop through an array instead. And generally speaking the setup should be in a
separate function.