Bug 226438

Summary: Add additional target luminance keywords for color-contrast() added for https://github.com/w3c/csswg-drafts/issues/6311
Product: WebKit Reporter: Sam Weinig <sam>
Component: CSSAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, macpherson, menard, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Comment 1 Sam Weinig 2021-05-30 15:59:29 PDT
Created attachment 430153 [details]
Patch
Comment 2 Darin Adler 2021-05-30 16:57:41 PDT
Comment on attachment 430153 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1618
> +                auto id = args.consumeIncludingWhitespace().id();
> +                for (auto& [identifier, luminance] : targetLuminanceMappings) {
> +                    if (identifier == id)
> +                        return luminance;
> +                }
> +                return std::nullopt;

Since these keys are sorted, you could have used SortedArrayMap and written this:

    constexpr SortedArrayMap targetLuminanceMap { targetLuminanceMappings };
    return targetLuminanceMap.tryGet(args.consumeIncludingWhitespace().id());

However, it returns a const double*, not a std::optional<double>. But in other ways it’s designed just for uses like this. Switches to binary search if the list gets long.
Comment 3 EWS 2021-05-30 17:52:26 PDT
Committed r278262 (238299@main): <https://commits.webkit.org/238299@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430153 [details].
Comment 4 Radar WebKit Bug Importer 2021-05-30 17:53:15 PDT
<rdar://problem/78670255>
Comment 5 Sam Weinig 2021-05-30 19:18:51 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 430153 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430153&action=review
> 
> > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1618
> > +                auto id = args.consumeIncludingWhitespace().id();
> > +                for (auto& [identifier, luminance] : targetLuminanceMappings) {
> > +                    if (identifier == id)
> > +                        return luminance;
> > +                }
> > +                return std::nullopt;
> 
> Since these keys are sorted, you could have used SortedArrayMap and written
> this:
> 
>     constexpr SortedArrayMap targetLuminanceMap { targetLuminanceMappings };
>     return targetLuminanceMap.tryGet(args.consumeIncludingWhitespace().id());
> 
> However, it returns a const double*, not a std::optional<double>. But in
> other ways it’s designed just for uses like this. Switches to binary search
> if the list gets long.

Missed this. Sorry. Addressed in https://bugs.webkit.org/show_bug.cgi?id=226444.