Bug 226438 - Add additional target luminance keywords for color-contrast() added for https://github.com/w3c/csswg-drafts/issues/6311
Summary: Add additional target luminance keywords for color-contrast() added for https...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-30 15:29 PDT by Sam Weinig
Modified: 2021-05-30 19:18 PDT (History)
9 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2021-05-30 15:59 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.