[webkit-reviews] review granted: [Bug 212184] Extended Color Cleanup: Stop allowing direct access to the underlying SimpleColor (it is almost always incorrect with respect to extended colors) : [Attachment 400148] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun May 24 03:06:03 PDT 2020


Dean Jackson <dino at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 212184: Extended Color Cleanup: Stop allowing direct access to the
underlying SimpleColor (it is almost always incorrect with respect to extended
colors)
https://bugs.webkit.org/show_bug.cgi?id=212184

Attachment 400148: Patch

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




--- Comment #20 from Dean Jackson <dino at apple.com> ---
Comment on attachment 400148
  --> https://bugs.webkit.org/attachment.cgi?id=400148
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1968
> +    auto color =
downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBASimpleColorLossy();
> +    r = color.redComponent();
> +    g = color.greenComponent();
> +    b = color.blueComponent();

Can you destructure this? If you had an "int a" that was discarded?

[r, g, b, a] =
downcast<HTMLInputElement>(*node()).valueAsColor().toSRGBASimpleColorLossy();

> Source/WebCore/platform/graphics/Color.cpp:182
>      // FIXME: This should probably return a floating point number, but many
of the call
>      // sites have picked comparison values based on feel. We'd need to break
out
>      // our logarithm tables to change them :)

Have you looked at the places that use this method? I wonder if the comment is
still true.

> Source/WebCore/platform/graphics/Color.cpp:433
> +    int r = (selfR * selfA * (255 - sourceA) + 255 * sourceA * sourceR) / d;
> +    int g = (selfG * selfA * (255 - sourceA) + 255 * sourceA * sourceG) / d;
> +    int b = (selfB * selfA * (255 - sourceA) + 255 * sourceA * sourceB) / d;

You use 0xff below, which I like.

> Source/WebCore/platform/graphics/Color.cpp:497
> +    uint8_t newAlpha = alpha * 255;

You use 0xff below, which I like.

Also, is there a reason you used uint8_t? Could we do it everywhere?

> Source/WebCore/platform/graphics/Color.cpp:509
> +	   return Color { extendedColor.red(), extendedColor.green(),
extendedColor.blue(), alpha, extendedColor.colorSpace() };

I wonder if we should have a constructor that is colorReplacingAlpha(const
Color& color, float alpha), or something like that (since we have
invertedColorWithAlpha)

> Source/WebCore/platform/graphics/Color.cpp:512
> +    // FIXME: This is where this function differs from colorWithAlphaUsing

Nit: missing period.

> Source/WebCore/platform/graphics/Color.cpp:526
> +    auto [r, g, b, existingAlpha] = rgb();

It's a bit confusing to me that a function called "rgb()" would return 4
components.

> Source/WebCore/platform/graphics/Color.h:72
> +    constexpr float alphaComponentAsFloat() const { return
static_cast<float>(alphaComponent()) / 0xFF; }

Nit: Lowercase 0xff to be consistent.

> Source/WebCore/platform/graphics/Color.h:81
> +    constexpr SimpleColor colorWithAlpha(uint8_t alpha) const { return {
(m_value & 0x00FFFFFF) | alpha << 24 }; }

Nit: Lowercase fs

> Source/WebCore/platform/graphics/ExtendedColor.cpp:66
> +Ref<ExtendedColor> ExtendedColor::invertedColorWithAlpha(float alpha) const

I wonder if we'll ever need to get color-nerdy and actually work out what we
mean by "inverted". Do we mean brightness? Won't inversion depend on the color
space?

> Source/WebCore/platform/graphics/ca/cocoa/PlatformCAAnimationCocoa.mm:399
> +    auto simpleColor = value.toSRGBASimpleColorLossy();

Another case where the destructuring would be nice. Since you're the template
wizard, is there a way to destructure into 3 components, ignoring the fourth?


More information about the webkit-reviews mailing list