[webkit-reviews] review granted: [Bug 174348] Improve use of NeverDestroyed : [Attachment 315595] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Darin Adler <darin at apple.com>'s request
for review:
Bug 174348: Improve use of NeverDestroyed
https://bugs.webkit.org/show_bug.cgi?id=174348

Attachment 315595: Patch

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




--- Comment #25 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 315595
  --> https://bugs.webkit.org/attachment.cgi?id=315595
Patch

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

Awesome! My biggest takeaway is that we have a lot of static maps / sets /
vectors that never change and that perhaps we should have specialized data
structures for them. And, it reaffirmed my curiosity on determining the right
size of a list deserves what data structure (e.g. does a list of 4 items really
need to be in a HashSet, or would lookups in Vector be just as fast).

> Source/WTF/wtf/NeverDestroyed.h:76
> +template<typename T> NeverDestroyed<T> makeNeverDestroyed(T&&);

I'm not sure I see the point of this forward declaration.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2868
> +    for (auto& expectedName : names) {
> +	   if (*expectedName == name)
> +	       return true;
> +    }
> +    return false;

This could be written as:

 std::any_of(names.begin(), names.end(), [&name] (auto& expectedName){ return
*expectedName == name; });

Not sure it's better.

If this idiom is common enough, maybe we should an overload of Vector::contains
that takes a predicate. (It kind of already exists in the form of
Vector::findMatching).

> Source/WebCore/editing/EditingStyle.cpp:901
> +static const Vector<const HTMLElementEquivalent*>& htmlElementEquivalents()

Maybe we need a std::array_ref
(http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3334.html) for things
like this. Or perhaps std::array could do it.

> Source/WebCore/loader/CrossOriginAccessControl.cpp:70
>  bool isOnAccessControlResponseHeaderWhitelist(const String& name)
>  {
> -    static std::once_flag onceFlag;
> -    static LazyNeverDestroyed<HTTPHeaderSet>
allowedCrossOriginResponseHeaders;
> -
> -    std::call_once(onceFlag, []{
> -	  
allowedCrossOriginResponseHeaders.construct<std::initializer_list<String>>({
> -	       "cache-control",
> -	       "content-language",
> -	       "content-type",
> -	       "expires",
> -	       "last-modified",
> -	       "pragma"
> -	   });
> +    static const auto allowedCrossOriginResponseHeaders =
makeNeverDestroyed(HTTPHeaderSet {
> +	   "cache-control",
> +	   "content-language",
> +	   "content-type",
> +	   "expires",
> +	   "last-modified",
> +	   "pragma"
>      });

This function seems to be unused. So you can just remove the whole thing.

> Source/WebCore/page/DebugPageOverlays.cpp:165
> +	   HashMap<String, Color> map;
> +	   for (auto& entry : entries)
> +	       map.add(entry.name, Color { entry.r, entry.g, entry.b, 80 });
> +	   return map;

Do initializer_lists for HashMaps like this not work? Something like:

map = { { "touchstart", Color(191, 191, 63, 80) }, ... }?

Would be nice if we could get that to work if it doesn't.

Also, elsewhere you do things like:

map.add(ASCIILiteral {entry.name }, ...). Might be good to be consistent.

> Source/WebCore/platform/MIMETypeRegistry.cpp:385
> +	   return { 1, type };

While the terseness is nice, I find this a very confusing construction. That
might be because I find the Vector constructor that it calls confusing. 
Perhaps something like { { type } } (if that compiles)?

> Source/WebCore/platform/SchemeRegistry.cpp:137
> +    static const auto schemes = makeNeverDestroyedStringVector({

Why not just makeNeverDestroyed(Vector<String> { ? Why the extra function here?

> Source/WebCore/platform/SchemeRegistry.cpp:151
> +    static auto secureSchemes =
makeNeverDestroyedSchemeSet(builtinSecureSchemes);

Seems like this could be build with an initialize_list as well.

> Source/WebCore/platform/graphics/FontCascade.cpp:497
> +    static const auto map = makeNeverDestroyed([] {
> +	   static const char* const fontFamiliesWithInvalidCharWidth[] = {

Can this just us an initializer_list directly?

> Source/WebCore/rendering/RenderThemeMac.mm:115
> + at interface NSTextFieldCell ()
> +- (CFDictionaryRef)_coreUIDrawOptionsWithFrame:(NSRect)cellFrame
inView:(NSView *)controlView includeFocus:(BOOL)includeFocus;
> + at end

This, a others in this file, should have a FIXME about moving it to one of the
..SPI.h files.

> Source/WebCore/rendering/svg/RenderSVGResource.cpp:178
> +	       static NeverDestroyed<HashSet<SVGElement*>>
invalidatingDependencies;

Ugh, this still bothers me that this is static.  Maybe it could live off
renderer.document().accessSVGExtensions()? Clearly moving it down here is an
improvement, as it shows much more clearly that the element is both added and
removed.

> Source/WebCore/svg/SVGPatternElement.cpp:103
> +	       SVGNames::patternUnitsAttr, SVGNames::patternContentUnitsAttr,
SVGNames::patternTransformAttr,
> +	       SVGNames::xAttr, SVGNames::yAttr, SVGNames::widthAttr,
SVGNames::heightAttr,

would put these each on their own line.