Bug 224512 – Use range-for loop in target contrast selection to simplify/improve code readability

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Use range-for loop in target contrast selection to simplify/improve code readability

[Sam Weinig]

Created attachment 425920 [details]
Patch

[Sam Weinig]

Couldn't work out a clear way to use std::max_element without duplicating luminance calculations or some ugly gymnastics.

[Darin Adler]

Comment on attachment 425920 [details]
Patch

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

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1512
> +static Color selectFirstColorThatMeetsOrExceedsTargetContrast(const Color& originBackgroundColor, Vector<Color>&& colorsToCompareAgainst, double targetContrast)

Annoying that this is double and the return value of WebCore::contrastRatio is float.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1519
>              return color;

Could use WTFMove(color) here since the vector is an rvalue reference. I presume that is why we wanted to pass ownership?

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1533
> +    auto* colorWithHigestContrastRatio = &colorsToCompareAgainst[0];

Just noticed "higest".

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1543
> +    return *colorWithHigestContrastRatio;

Could use WTFMove(*colorWithHighestContrastRatio) here since the vector is an rvalue reference. I presume that is why we wanted to pass ownership?

[Darin Adler]

Separately looked at callers of contrastRatio. Noticed this:

    // Uses WCAG 2.0 definition of legibility: contrast ratio of 4.5:1 or greater.
    // https://www.w3.org/TR/WCAG20/#visual-audio-contrast-contrast
    return Color::contrastRatio(textColor, backgroundColor) > 4.5;

Comment says >=, code says >. And contrastRatio returns float, but we compare with a double.

Also not sure that contrastRatio should be a static member of the Color class. Just overload it for Color arguments but keep it at the WebCore level since callOnUnderlyingType is a public member function template.

Also don’t need to write WebCore::contrastRatio in most places. Just contrastRatio will do. Also don’t need a local variable named contrastRatio in selectFirstColorThatMeetsOrExceedsTargetContrast; local is only used once.

Also could use the phrase "highest contrast" or "greatest contrast" instead of "highest contrast ratio" to keep variable names smaller.

Also the implementation of contrast ratio converts from float to double and back because of the use of "0.05" instead of "0.05f".

[Sam Weinig]

Created attachment 425937 [details]
Patch

[Sam Weinig]

Address more feedback in this last one.

[Sam Weinig]

Created attachment 425939 [details]
Patch

[Darin Adler]

Comment on attachment 425939 [details]
Patch

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

> Source/WebCore/rendering/TextPaintStyle.cpp:66
>      // Uses the WCAG 2.0 definition of legibility: a contrast ratio of 4.5:1 or greater.
>      // https://www.w3.org/TR/WCAG20/#visual-audio-contrast-contrast
> -    return Color::contrastRatio(textColor, backgroundColor) > 4.5;
> +    return contrastRatio(textColor, backgroundColor) > 4.5;

Comment says >=, code still says just >. I know this is floating point, so the difference is infinitesimal, but still ...

[Sam Weinig]

Created attachment 425990 [details]
Patch

[Darin Adler]

Comment on attachment 425990 [details]
Patch

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

Really like these refinements. And I do have a parting shot.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1514
> -static Color selectFirstColorThatMeetsOrExceedsTargetContrast(const Color& originBackgroundColor, const Vector<Color>& colorsToCompareAgainst, double targetContrast)
> +static Color selectFirstColorThatMeetsOrExceedsTargetContrast(const Color& originBackgroundColor, Vector<Color>&& colorsToCompareAgainst, double targetContrast)
>  {
>      auto originBackgroundColorLuminance = originBackgroundColor.luminance();

Too late for this patch, and eventually we hit diminishing returns, but please indulge me with one more thought from the "Darin coding manual of style": The names here are too long.

The function uses "origin background color" as an argument name; that is unmotivated. This function is not about "origins" so the word origin is intruding from the context where the function is used, not so appropriate within the function itself, unless something about the function was truly related to the notion of an origin.

And this local variable hardly needs so many words in its name given the context of this short function. I would probably call it "backgroundLuminance", or if no one was watching me and I left to my own devices I might even call it "luminance" (and be objectively wrong).

I feel better about "colorsToCompareAgainst"; additional context in that name seems welcome even for this short function. But note that we not only *compare* against those colors but rather we *select* from among them, which explains why this function takes ownership. So even that name might be refined. I might name that foregroundColors or foregroundColorCandidates.

> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1528
> +static Color selectFirstColorWithHighestContrast(const Color& originBackgroundColor, Vector<Color>&& colorsToCompareAgainst)

Same naming issues.

[EWS]

Found 1 new test failure: webrtc/peerconnection-new-candidate-page-cache.html

[EWS]

Found 1 new test failure: http/tests/webgl/1.0.2/origin-clean-conformance.html

[EWS]

Committed r275981 (236533@main): <https://commits.webkit.org/236533@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425990 [details].

[Radar WebKit Bug Importer]

<rdar://problem/76673550>