Bug 222110 – color(lab ...) should serialize as color(lab ...) not lab() according to latest CSS Color 4 spec

• Component: CSS
• Status: RESOLVED FIXED

[Sam Weinig]

color(lab ...) should serialize as color(lab ...) not lab() according to latest CSS Color 4 spec.

[Sam Weinig]

I'm going to take this opportunity to add some new functionality to WebCore::Color to have it store a bit saying whether to use the color(...) serialization or default serialization for an underlying color type. 

For some color types (like DisplayP3, for instance), the default is the color(...) serialization, so this bit won't have any effect, but it will allow us to do the optimization I have been wanting to do for a while to store color(srgb ...) colors that can be represented as SRGBA<uint8_t> losslessly in the efficient form and still retain the correct serialization.

[Sam Weinig]

Created attachment 420909 [details]
Patch

[Sam Weinig]

Created attachment 420911 [details]
Patch

[Sam Weinig]

I'm not sure how I feel about this change. It turned out bigger than I anticipated.

[Dean Jackson]

Comment on attachment 420911 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.cpp:129
> +            auto result = colorWithOverridenAlpha(underlyingColor, alpha);

Not new in this patch, but there is a typo in Overridden.

> Source/WebCore/platform/graphics/Color.h:198
> +    enum class AllFlags {

It is common to have this All* enum? I would have thought you call this one Flags and the earlier one FlagsSubset or something?

[Darin Adler]

Comment on attachment 420911 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.h:85
> +    Color(SRGBA<uint8_t>, OptionSet<Flags>);
> +    Color(Optional<SRGBA<uint8_t>>, OptionSet<Flags>);

Could we use default values for the flags to reduce the total number of constructors? Would not want to sacrifice runtime efficiency, but would love to have the list be much shorter.

[Darin Adler]

(In reply to Sam Weinig from comment #1)
> I'm going to take this opportunity to add some new functionality to
> WebCore::Color to have it store a bit saying whether to use the color(...)
> serialization or default serialization for an underlying color type. 
> 
> For some color types (like DisplayP3, for instance), the default is the
> color(...) serialization, so this bit won't have any effect, but it will
> allow us to do the optimization I have been wanting to do for a while to
> store color(srgb ...) colors that can be represented as SRGBA<uint8_t>
> losslessly in the efficient form and still retain the correct serialization.

I don’t understand why this is separate state in the Color object rather than a property of the color space.

[Sam Weinig]

(In reply to Darin Adler from comment #7)
> (In reply to Sam Weinig from comment #1)
> > I'm going to take this opportunity to add some new functionality to
> > WebCore::Color to have it store a bit saying whether to use the color(...)
> > serialization or default serialization for an underlying color type. 
> > 
> > For some color types (like DisplayP3, for instance), the default is the
> > color(...) serialization, so this bit won't have any effect, but it will
> > allow us to do the optimization I have been wanting to do for a while to
> > store color(srgb ...) colors that can be represented as SRGBA<uint8_t>
> > losslessly in the efficient form and still retain the correct serialization.
> 
> I don’t understand why this is separate state in the Color object rather
> than a property of the color space.

Currently, color spaces don't have any additional state, so I used the fact that Color does have extra state and serialization always goes through Color to store it.

Another approach would be to add new values to the ColorSpace enum (and unfortunately new color types as well, since we map from color type to ColorSpace enum) for the colors that can have an alternative serialization.

For this, that would mean adding a ColorSpace::LabUsingColorSerialization and a LabUsingColorSerialization<float> type. Not horrible, but also not ideal when we can just use a bit from Color.

[Sam Weinig]

(In reply to Darin Adler from comment #6)
> Comment on attachment 420911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420911&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:85
> > +    Color(SRGBA<uint8_t>, OptionSet<Flags>);
> > +    Color(Optional<SRGBA<uint8_t>>, OptionSet<Flags>);
> 
> Could we use default values for the flags to reduce the total number of
> constructors? Would not want to sacrifice runtime efficiency, but would love
> to have the list be much shorter.

Yes, we can. I think it is extremely unlikely to cause any runtime change in release builds at all. Will fix.

[Sam Weinig]

(In reply to Dean Jackson from comment #5)
> Comment on attachment 420911 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420911&action=review
> 
> > Source/WebCore/platform/graphics/Color.cpp:129
> > +            auto result = colorWithOverridenAlpha(underlyingColor, alpha);
> 
> Not new in this patch, but there is a typo in Overridden.

:). Will fix.

> 
> > Source/WebCore/platform/graphics/Color.h:198
> > +    enum class AllFlags {
> 
> It is common to have this All* enum? I would have thought you call this one
> Flags and the earlier one FlagsSubset or something?

I want the public enum to be called Flags, so this one had to be called something else. I agree AllFlags is not perfect. Maybe FlagsAndInternalState?

[Darin Adler]

I would choose the name FlagsIncludingInternal or since it’s C++ and the language calls it "private", FlagsIncludingPrivate.

[Darin Adler]

Irritating that we can’t use inheritance or anything like that. For the common flags I would at least suggest using the same symbolic constants to prove they match. Not sure how to check that the private flags don’t accidentally overlap the public.

[Sam Weinig]

(In reply to Darin Adler from comment #12)
> Irritating that we can’t use inheritance or anything like that. For the
> common flags I would at least suggest using the same symbolic constants to
> prove they match. Not sure how to check that the private flags don’t
> accidentally overlap the public.

I do currently do a static_assert for the common ones:

+   static_assert(static_cast<unsigned>(Flags::Semantic) == static_cast<unsigned>(AllFlags::Semantic));
+   static_assert(static_cast<unsigned>(Flags::UseColorFunctionSerialization) == static_cast<unsigned>(AllFlags::UseColorFunctionSerialization));

Did you mean something past that?

Another options might be to fake it like this:


```
class Color {
public:
    struct Flags {
        enum Flag : uint16_t;
        static constexpr Flag Semantic                      = 1 << 0;
        static constexpr Flag UseColorFunctionSerialization = 1 << 1;
    };

...
private:
    struct FlagsIncludingPrivate : Flags {
        static constexpr Flag Valid                         = 1 << 2;
        static constexpr Flag Extended                      = 1 << 3;
        static constexpr Flag HashTableEmptyValue           = 1 << 4;
        static constexpr Flag HashTableDeletedValue         = 1 << 5;
    };
```

And then I think I could use OptionSet<Flags::Flag> everywhere, but only internally would the additional names be available.

[Dean Jackson]

(In reply to Darin Adler from comment #12)
> Irritating that we can’t use inheritance or anything like that.

Time to adopt Swift!!

[Dean Jackson]

Comment on attachment 420911 [details]
Patch

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

> Source/WebCore/platform/graphics/Color.cpp:127
>      return callOnUnderlyingType(WTF::makeVisitor(

I want to make this comment because I felt smart for finding it, but also dumb for not being confident in saying it..... you don't need a visitor any more since you now only have one function.

[Sam Weinig]

Created attachment 421092 [details]
Patch

[EWS]

Committed r273211: <https://commits.webkit.org/r273211>

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

[Radar WebKit Bug Importer]

<rdar://problem/74560531>

[Simon Fraser (smfr)]

When you change Color data layout please also remember to fix Tools/lldb/lldb_webkit.py