Bug 212184 – Extended Color Cleanup: Stop allowing direct access to the underlying SimpleColor (it is almost always incorrect with respect to extended colors)

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

[Sam Weinig]

Extended Color Cleanup: Make Color::rgb() private

[Sam Weinig]

Created attachment 399926 [details]
Patch

[Sam Weinig]

Created attachment 399929 [details]
Patch

[Sam Weinig]

Created attachment 399949 [details]
Patch

[Sam Weinig]

Created attachment 399969 [details]
Patch

[Sam Weinig]

Not ready for real review yet but this change does a few things all in the name of making Color::rgb() private:

- Replaces a weird common idiom I don't quite understand the reasoning of where a Color was created out of another color.rgb():
    OLD: Color(myColor.rgb())
    NEW: myColor
 Best I can tell, this would just drop the Semantic bit and is just broken completely with ExtendedColor

- Replaces colorWithOverrideAlpha(RGBA) with a Color:: colorWithAlphaUsingAlternativeRounding(). This variant works with ExtendedColors and makes it clear we need to reconcile the two variants (just not in this patch, as it will require changing test results)

- Makes premultipliedARGBFromColor/colorFromPremultipliedARGB work strictly with RGBA32s, as they don't make any sense with ExtendedColor as you lose the ColorSpace information. The one remaining bit on Color itself is a new premultipliedARGB() function, which is only used by TextureMapperGL.cpp, and should be replaced with instead premultiplying in FloatComponents instead.

- Move IPC coders from WebKit to Color via the normal template encoded/decoder pattern.

- Other easier removals of rgb() usage where equivalent means existed.


One straggler is a change in RenderThemeIOS::systemColor(), where the code was taking a Color from a global map and creating a new Color as return Color(it->value.rgb(), Color::Semantic), essentially adding the Semantic bit to it. I am not sure exactly why this is needed, but rather than removing it, in the next iteration here, I will probably add a new function in Color which returns the same Color with the Semantic bit set, and leave this mystery for another time. (One guess I have is that that the Colors in this map were coming over IPC from the UI process and were losing their Semantic bit, as that is not included in the IPC currently. If so, the solution would be to encode that bit, and drop this silliness).

[Sam Weinig]

Created attachment 399972 [details]
Patch

[Sam Weinig]

Created attachment 399976 [details]
Patch

[Sam Weinig]

Split out a chunk of this into https://bugs.webkit.org/show_bug.cgi?id=212231

[Sam Weinig]

Split out another chunk in https://bugs.webkit.org/show_bug.cgi?id=212184

[Sam Weinig]

Split out another chunk in https://bugs.webkit.org/show_bug.cgi?id=212265

[Sam Weinig]

Created attachment 400130 [details]
Patch

[Sam Weinig]

Created attachment 400134 [details]
Patch

[Sam Weinig]

Created attachment 400136 [details]
Patch

[Sam Weinig]

Created attachment 400138 [details]
Patch

[Sam Weinig]

Created attachment 400139 [details]
Patch

[Sam Weinig]

Created attachment 400143 [details]
Patch

[Sam Weinig]

Should be ready for review now.

[Sam Weinig]

Created attachment 400144 [details]
Patch

[Sam Weinig]

Created attachment 400148 [details]
Patch

[Dean Jackson]

Comment on attachment 400148 [details]
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?

[Sam Weinig]

(In reply to Dean Jackson from comment #20)
> Comment on attachment 400148 [details]
> 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();

There is no explicit syntax in c++ yet for discarding a value (it's being discussed a lot in the standards body) but you can just not use one of the variables. In this case, I did,'t use destructuring since the out parameters the function has made it awkward.

> 
> > 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.

I have not. Worth a followup.

> 
> > 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.

Changed. Though I used 0xFF since that is more prevalent in the file (also changed the one place I used 0xff).

> 
> > Source/WebCore/platform/graphics/Color.cpp:497
> > +    uint8_t newAlpha = alpha * 255;
> 
> You use 0xff below, which I like.

Changed.

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

Yeah, we should use it more consistently.

> 
> > 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)

Will be doing more of this in a follow up.

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

Fixed.

> 
> > 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.

Yeah. This function will get a name change soon.

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

Kept it 0xFF as that is more prevalent in this file.

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

Kept it 0xFF as that is more prevalent in this file.

> 
> > 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?

Probably :(. There are a bunch of weird things we do around selection including this insert and the blendWithWhite() function that probably need to be reconsidered.

> 
> > 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?

Fixed.

[Sam Weinig]

Created attachment 400161 [details]
Patch

[EWS]

Committed r262108: <https://trac.webkit.org/changeset/262108>

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

[Radar WebKit Bug Importer]

<rdar://problem/63579502>