Bug 216293

Created attachment 408287 [details]
Patch

Comment on attachment 408287 [details]
Patch

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

> Source/WebCore/rendering/RenderTheme.cpp:1286
> +#define NumSystemFonts 10

This can be constexpr; does not need to be "define":

    constexpr auto numSystemFonts = 10;

> Source/WebCore/rendering/RenderTheme.cpp:1288
> +        FontCascadeDescription& caption()            { return (*this)[0]; }

Seems to me you could just use an array here and use indices in the code below and don’t need the named getters.

> Source/WebCore/rendering/RenderTheme.cpp:1298
> +        static_assert(NumSystemFonts == 10);

This seems pointless given the number is defined just above.

> Source/WebCore/rendering/RenderThemeCocoa.mm:209
> +            for (auto& description : (*this)) {
> +                if (description.isAbsoluteSize())
> +                    return false;
> +            }
> +            return true;

Could write this with std::all_of or none_of:

    return std:: all_of(std::begin(descriptions), std::end(descriptions), [] (auto& description) (
        return !description.isAbsoluteSize();
    });

Created attachment 408304 [details]
Patch

Comment on attachment 408304 [details]
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.

Committed r266904: <https://trac.webkit.org/changeset/266904>

<rdar://problem/68679053>