Bug 214204 – Reduce surface area of the ExtendedColor class to a bare minimum

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Reduce surface area of the ExtendedColor class to a bare minimum

[Sam Weinig]

Created attachment 404014 [details]
Patch

[Sam Weinig]

Created attachment 404021 [details]
Patch

[Sam Weinig]

Created attachment 404023 [details]
Patch

[Sam Weinig]

Created attachment 404033 [details]
Patch

[Sam Weinig]

Created attachment 404035 [details]
Patch

[Darin Adler]

Comment on attachment 404033 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:56
> +//    - Special "invalid color" state

maybe saying "treated as transparent black but distinguishable"

> Source/WebCore/platform/graphics/Color.h:102
>      bool isSemantic() const { return isInline() && (m_colorData.inlineColorAndFlags & isSemanticInlineColorBit); }
>  
> +
>      bool isOpaque() const { return isExtended() ? asExtended().alpha() == 1.0 : asInline().alpha == 255; }

Not a fan of the various double spaces; I don’t think they really help this much.

> Source/WebCore/platform/graphics/Color.h:117
> +    WEBCORE_EXPORT std::pair<ColorSpace, ColorComponents<float>> colorSpaceAndComponents() const;

Better as a tuple than as a struct (food for future thought)?

> Source/WebCore/platform/graphics/Color.h:172
> +    static bool isBlack(const Color&);
> +    static bool isWhite(const Color&);

These functions have unclear semantics. Why are they better than "== Color::black"?

> Source/WebCore/platform/graphics/Color.h:287
> +}

ASSERT_NOT_REACHED()? Or is invalid color a good outcome?

> Source/WebCore/platform/graphics/Color.h:336
> +        auto [c1, c2, c3, alpha] = asExtended().components();
> +        return computeHash(c1, c2, c3, alpha, asExtended().colorSpace());

I think computeHash already knows how to hash tuples, so this can be:

    return computeHash(asExtended().components(), asExtended().colorSpace());

> Source/WebCore/platform/graphics/Color.h:410
> +    auto converted = convertToComponentBytes(color);
> +    if (color == convertToComponentFloats(converted)) {
> +        setColor(converted);
> +        return;
> +    }

Big new behavior! This has some consequences after this point, for the many Color functions that do subtly different operations on simple and extended colors.

> Source/WebCore/platform/graphics/ColorSerialization.cpp:106
>      if (WTF::areEssentiallyEqual(alpha, 1.0f))

Why does this use areEssentiallyEqual instead of ==? Does not seem motivated.

> Source/WebCore/platform/graphics/ExtendedColor.h:37
>  class ExtendedColor : public RefCounted<ExtendedColor> {

Seems like should become a private member class of Color if you finish the job of trimming it down like this.

> Tools/ChangeLog:8
> +        FIXME: DO NOT LAND UNTIL ADDING NEW TESTS FOR NEW BEHAVIOR OF STORING REPRESENTABLE SRGBA<float> INLINE.

Ah, the "big new behavior" comment apparently is something not lost on you.

Why not leave out that one bit of code and land it separately from this refactoring? It’s not intertwined with the rest.

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 404033 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404033&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:56
> > +//    - Special "invalid color" state
> 
> maybe saying "treated as transparent black but distinguishable"
> 
> > Source/WebCore/platform/graphics/Color.h:102
> >      bool isSemantic() const { return isInline() && (m_colorData.inlineColorAndFlags & isSemanticInlineColorBit); }
> >  
> > +
> >      bool isOpaque() const { return isExtended() ? asExtended().alpha() == 1.0 : asInline().alpha == 255; }
> 
> Not a fan of the various double spaces; I don’t think they really help this
> much.

Yeah, an attempt was made, but it didn't work. Will remove them.

> 
> > Source/WebCore/platform/graphics/Color.h:117
> > +    WEBCORE_EXPORT std::pair<ColorSpace, ColorComponents<float>> colorSpaceAndComponents() const;
> 
> Better as a tuple than as a struct (food for future thought)?

Yes.

> 
> > Source/WebCore/platform/graphics/Color.h:172
> > +    static bool isBlack(const Color&);
> > +    static bool isWhite(const Color&);
> 
> These functions have unclear semantics. Why are they better than "==
> Color::black"?

The difference from == Color::black, is that it it will return true for non-sRGB colors. The implementation is really asking, are all the components at there maximum (or minimum for Color::isWhite). That is not necessarily the same color as Color::black for, say, DisplayP3 colors.

All that said, the real question is what do the callers want. There are only a couple callers of these functions, so let's try to figure out what they are really after:

1) Document::compositeOperatorForBackgroundColor() calls Color::isWhite(). It seems like what this code is trying to do is determine if it needs to comply with the punchOutWhiteBackgroundsInDarkMode() setting. In this case, I think color == Color::white is probably ok. If a user sets the background color of message to color(display-p3 0 0 0), it's probably ok not to punch out, just like if they set their background to color: rgb(0, 0, 1). My interpretation of the use of isWhite() here is really trying to determine if the background color is the default background color, which is Color::white. Perhaps there is actually a better way to determine that?

2) normalizedColor in HTMLConverter.cpp. Once again about dark mode. I think a similar analysis as for #1 can be made here, and that using == Color::white or == Color::black would probably be sufficient.

3) RenderLayerBacking::shouldDumpPropertyForLayer calls Color::isWhite. This one definitely can use == Color::white. This is just to not dump background colors when they are white. If it's a non-sRGB white, that's probably interesting enough to dump.

4) Editor::fontAttributesAtSelectionStart calls Color::isBlack. This one seems to be about whether we set foregroundColor font attribute. I think Color::black would be correct here as well, as a non-sRGB black might be something we want to preserve in the attributes.

So, in conclusion, yeah, we should probably get rid of Color::isBlack/Color::isWhite.

Thoughts?


> 
> > Source/WebCore/platform/graphics/Color.h:287
> > +}
> 
> ASSERT_NOT_REACHED()? Or is invalid color a good outcome?

I didn't add the ASSERT_NOT_REACHED() because if other color spaces are added, it will be a compile failure (due to no default case in the switch). 

> 
> > Source/WebCore/platform/graphics/Color.h:336
> > +        auto [c1, c2, c3, alpha] = asExtended().components();
> > +        return computeHash(c1, c2, c3, alpha, asExtended().colorSpace());
> 
> I think computeHash already knows how to hash tuples, so this can be:
> 
>     return computeHash(asExtended().components(), asExtended().colorSpace());

Very cool.

> 
> > Source/WebCore/platform/graphics/Color.h:410
> > +    auto converted = convertToComponentBytes(color);
> > +    if (color == convertToComponentFloats(converted)) {
> > +        setColor(converted);
> > +        return;
> > +    }
> 
> Big new behavior! This has some consequences after this point, for the many
> Color functions that do subtly different operations on simple and extended
> colors.

Yeah, going to rip this out of this change. Asked Simon and he says we have to preserve the current serializations. So:

    color: color(srgb 1 1 1); 

has to serialize to itself, and not:

    color: rgb(255, 255, 255); 

which is what this change does. So, if we do bring this back, we will have to keep a bit in Color indicating it should use the color() serialization (unless we can start serializing all colors as color(srgb 1 1 1), Simon has not gotten back to me on that).
    c

> 
> > Source/WebCore/platform/graphics/ColorSerialization.cpp:106
> >      if (WTF::areEssentiallyEqual(alpha, 1.0f))
> 
> Why does this use areEssentiallyEqual instead of ==? Does not seem motivated.

Not sure, but also not new, so I am going to leave it alone for this change.

> 
> > Source/WebCore/platform/graphics/ExtendedColor.h:37
> >  class ExtendedColor : public RefCounted<ExtendedColor> {
> 
> Seems like should become a private member class of Color if you finish the
> job of trimming it down like this.

Yep. 

> 
> > Tools/ChangeLog:8
> > +        FIXME: DO NOT LAND UNTIL ADDING NEW TESTS FOR NEW BEHAVIOR OF STORING REPRESENTABLE SRGBA<float> INLINE.
> 
> Ah, the "big new behavior" comment apparently is something not lost on you.
> 
> Why not leave out that one bit of code and land it separately from this
> refactoring? It’s not intertwined with the rest.

Yeah, it's going.

(I didn't mean for this to be up for review yet, I just forgot to add --no-review to my webkit-patch post, but non-the-less, thank you for the review).

[Sam Weinig]

Created attachment 404038 [details]
Patch

[Sam Weinig]

Created attachment 404039 [details]
Patch

[Sam Weinig]

(In reply to Sam Weinig from comment #7)
> (In reply to Darin Adler from comment #6)
> > Comment on attachment 404033 [details]
> > Patch
> > 
> > > Source/WebCore/platform/graphics/Color.h:336
> > > +        auto [c1, c2, c3, alpha] = asExtended().components();
> > > +        return computeHash(c1, c2, c3, alpha, asExtended().colorSpace());
> > 
> > I think computeHash already knows how to hash tuples, so this can be:
> > 
> >     return computeHash(asExtended().components(), asExtended().colorSpace());
> 
> Very cool.

Looks like it's a bit too strict about how tuple-ish the tuple needs to be. Going to leave it out of this, but will try to fix that up to see if I can make it work with any destructurable object.

[Sam Weinig]

Comment on attachment 404039 [details]
Patch

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

> Source/WebCore/ChangeLog:35
> +        - 

I'll remove this before landed.

[Sam Weinig]

(In reply to Sam Weinig from comment #10)
> (In reply to Sam Weinig from comment #7)
> > (In reply to Darin Adler from comment #6)
> > > Comment on attachment 404033 [details]
> > > Patch
> > > 
> > > > Source/WebCore/platform/graphics/Color.h:336
> > > > +        auto [c1, c2, c3, alpha] = asExtended().components();
> > > > +        return computeHash(c1, c2, c3, alpha, asExtended().colorSpace());
> > > 
> > > I think computeHash already knows how to hash tuples, so this can be:
> > > 
> > >     return computeHash(asExtended().components(), asExtended().colorSpace());
> > 
> > Very cool.
> 
> Looks like it's a bit too strict about how tuple-ish the tuple needs to be.
> Going to leave it out of this, but will try to fix that up to see if I can
> make it work with any destructurable object.

Added support to computeHash in https://bugs.webkit.org/show_bug.cgi?id=214224.

[Sam Weinig]

Created attachment 404072 [details]
Patch

[Darin Adler]

Comment on attachment 404072 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:340
> +    return functor(asInline());

Wonder what determines if we should call the function like this or use std::invoke/std::forward. I am guessing std::invoke works on a slightly larger variety of functors?

> Source/WebCore/platform/graphics/ColorSerialization.cpp:167
> +    return color.callOnUnderlyingType([] (auto c) {

Would have preferred a word rather than the letter "c".

> Source/WebCore/platform/graphics/ColorSerialization.cpp:174
> +    return color.callOnUnderlyingType([] (auto c) {

Ditto.

> Source/WebCore/platform/graphics/ColorSerialization.cpp:181
> +    return color.callOnUnderlyingType([] (auto c) {

Ditto.

> Source/WebCore/platform/graphics/ColorTypes.h:33
> +template<typename T> struct ComponentTraits;

No need for the "T" here.

> Source/WebCore/platform/graphics/ColorTypes.h:53
> +    static constexpr ColorSpace colorSpace = ColorSpace::SRGB;

I would do "constexpr auto" instead of "constexpr ColorSpace".

[Sam Weinig]

(In reply to Darin Adler from comment #14)
> Comment on attachment 404072 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404072&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:340
> > +    return functor(asInline());
> 
> Wonder what determines if we should call the function like this or use
> std::invoke/std::forward. I am guessing std::invoke works on a slightly
> larger variety of functors?

Yeah, we should probably use invoke here (and probably in more generic contexts). Will fix.

> 
> > Source/WebCore/platform/graphics/ColorSerialization.cpp:167
> > +    return color.callOnUnderlyingType([] (auto c) {
> 
> Would have preferred a word rather than the letter "c".

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorSerialization.cpp:174
> > +    return color.callOnUnderlyingType([] (auto c) {
> 
> Ditto.

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorSerialization.cpp:181
> > +    return color.callOnUnderlyingType([] (auto c) {
> 
> Ditto.

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:33
> > +template<typename T> struct ComponentTraits;
> 
> No need for the "T" here.

Fixed.

> 
> > Source/WebCore/platform/graphics/ColorTypes.h:53
> > +    static constexpr ColorSpace colorSpace = ColorSpace::SRGB;
> 
> I would do "constexpr auto" instead of "constexpr ColorSpace".

Fixed.

[Sam Weinig]

Created attachment 404075 [details]
Patch

[EWS]

Committed r264272: <https://trac.webkit.org/changeset/264272>

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

[Radar WebKit Bug Importer]

<rdar://problem/65414170>

[Fujii Hironori]

Committed r264289: <https://trac.webkit.org/changeset/264289>

[Darin Adler]

I’m not sure moving the include to the header was the best fix. We might have been able to just forward declare in the header and include only in the files that were using that template.

[Fujii Hironori]

(In reply to Darin Adler from comment #20)
> I’m not sure moving the include to the header was the best fix. We might
> have been able to just forward declare in the header and include only in the
> files that were using that template.

I'm surprised by your comment.
WebCore has a lot of template functions and template classes, but less forward declarations for them.
Is it really common idiom in WebCore?

In this case ComponentTraits is used only in ColorUtilities.h. It can be moved to ColorUtilities.h.

[Darin Adler]

(In reply to Fujii Hironori from comment #21)
> (In reply to Darin Adler from comment #20)
> > I’m not sure moving the include to the header was the best fix. We might
> > have been able to just forward declare in the header and include only in the
> > files that were using that template.
> 
> I'm surprised by your comment.
> WebCore has a lot of template functions and template classes, but less
> forward declarations for them.
> Is it really common idiom in WebCore?

Maybe not. But forward declaring whenever we can *is* common in WebCore, we may not be doing it as much as we need with templates. Note that template functions don't always require all their prerequisites just because a header is included. Instantiating the function then requires the prerequisites. That’s how I understand it at least.

Faster building requires including the minimum and reducing includes.

[Fujii Hironori]

(In reply to Darin Adler from comment #22)
> Maybe not. But forward declaring whenever we can *is* common in WebCore, we
> may not be doing it as much as we need with templates. Note that template
> functions don't always require all their prerequisites just because a header
> is included. Instantiating the function then requires the prerequisites.
> That’s how I understand it at least.

OK. Forward template declarations can be useful for the case of
template funcitons and the case of using the pointer or reference
of template classes.

In this case, ComponentTraits is a template class, and ColorUtilities.h is using ComponentTraits members.
Thus, ColorUtilities.h needs ComponentTraits definition.

[Darin Adler]

(In reply to Fujii Hironori from comment #23)
> (In reply to Darin Adler from comment #22)
> In this case, ComponentTraits is a template class, and ColorUtilities.h is
> using ComponentTraits members.
> Thus, ColorUtilities.h needs ComponentTraits definition.

Is that right?

ComponentTraits members are used inside *function templates* in ColorUtilities.h. Does that really mean the class template needs to be *defined* when ColorUtilities.h is included but those function templates are not instantiated?

[Fujii Hironori]

(In reply to Darin Adler from comment #24)
> (In reply to Fujii Hironori from comment #23)
> > (In reply to Darin Adler from comment #22)
> > In this case, ComponentTraits is a template class, and ColorUtilities.h is
> > using ComponentTraits members.
> > Thus, ColorUtilities.h needs ComponentTraits definition.
> 
> Is that right?
> 
> ComponentTraits members are used inside *function templates* in
> ColorUtilities.h. Does that really mean the class template needs to be
> *defined* when ColorUtilities.h is included but those function templates are
> not instantiated?

You are right. Will fix it.

[Fujii Hironori]

Committed r264588: <https://trac.webkit.org/changeset/264588>

[Fujii Hironori]

Committed r264589: <https://trac.webkit.org/changeset/264589>

[Fujii Hironori]

(In reply to Fujii Hironori from comment #25)
> You are right. Will fix it.

Filed: Bug 214540 – Use forward template declarations for ColorComponents instead of including ColorComponents.h