[webkit-reviews] review granted: [Bug 216293] Small cleanup in RenderTheme : [Attachment 408304] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 216293: Small cleanup in RenderTheme
https://bugs.webkit.org/show_bug.cgi?id=216293

Attachment 408304: Patch

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




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

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

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:82
> +    auto atomStringComparison = [](const AtomString& lhs, const AtomString&
rhs) {

I think this name doesn’t say the thing it should, which is that this is the
most-efficient comparison, comparison by pointer, with the drawback that it
sorts things in pseudo-random, non-useful order. I think I would call this
compareAsPointer; no need to state the types of the arguments. But maybe you
have a different idea about the name. The name atomStringComparison would be a
fine name for comparing the string by code point, or code unit, or case folded
ASCII, or whatever, and none of those  are what want to use here.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:85
> +    static auto strings { makeNeverDestroyed([&atomStringComparison] {

Not 100% sure about the { } vs. = for the initialization, what is inside the
lambda and what is outside, and other such things. I would have been tempted to
write "const", but I suppose that’s not needed.

> Source/WebCore/platform/graphics/cocoa/FontDescriptionCocoa.cpp:91
> +    auto result = std::binary_search(strings.get().begin(),
strings.get().end(), string, atomStringComparison);
> +    if (result)

No need for a local variable "result" here.