Bug 222110 - color(lab ...) should serialize as color(lab ...) not lab() according to latest CSS Color 4 spec
Summary: color(lab ...) should serialize as color(lab ...) not lab() according to late...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-18 08:55 PST by Sam Weinig
Modified: 2021-03-17 21:48 PDT (History)
13 users (show)

See Also:


Attachments
Patch (57.45 KB, patch)
2021-02-18 20:03 PST, Sam Weinig
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (57.54 KB, patch)
2021-02-18 20:09 PST, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (62.75 KB, patch)
2021-02-20 10:34 PST, Sam Weinig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2021-02-18 08:55:55 PST
color(lab ...) should serialize as color(lab ...) not lab() according to latest CSS Color 4 spec.
Comment 1 Sam Weinig 2021-02-18 08:59:20 PST
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.
Comment 2 Sam Weinig 2021-02-18 20:03:03 PST
Created attachment 420909 [details]
Patch
Comment 3 Sam Weinig 2021-02-18 20:09:47 PST
Created attachment 420911 [details]
Patch
Comment 4 Sam Weinig 2021-02-18 20:10:22 PST
I'm not sure how I feel about this change. It turned out bigger than I anticipated.
Comment 5 Dean Jackson 2021-02-19 04:05:30 PST
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?
Comment 6 Darin Adler 2021-02-19 08:52:14 PST
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.
Comment 7 Darin Adler 2021-02-19 08:52:54 PST
(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.
Comment 8 Sam Weinig 2021-02-19 11:04:11 PST
(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.
Comment 9 Sam Weinig 2021-02-19 11:05:04 PST
(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.
Comment 10 Sam Weinig 2021-02-19 11:06:28 PST
(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?
Comment 11 Darin Adler 2021-02-19 11:56:17 PST
I would choose the name FlagsIncludingInternal or since it’s C++ and the language calls it "private", FlagsIncludingPrivate.
Comment 12 Darin Adler 2021-02-19 11:57:25 PST
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.
Comment 13 Sam Weinig 2021-02-19 12:31:44 PST
(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.
Comment 14 Dean Jackson 2021-02-19 13:12:15 PST
(In reply to Darin Adler from comment #12)
> Irritating that we can’t use inheritance or anything like that.

Time to adopt Swift!!
Comment 15 Dean Jackson 2021-02-19 13:13:51 PST
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.
Comment 16 Sam Weinig 2021-02-20 10:34:48 PST
Created attachment 421092 [details]
Patch
Comment 17 EWS 2021-02-20 16:48:06 PST
Committed r273211: <https://commits.webkit.org/r273211>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421092 [details].
Comment 18 Radar WebKit Bug Importer 2021-02-20 16:49:14 PST
<rdar://problem/74560531>
Comment 19 Simon Fraser (smfr) 2021-03-17 21:48:14 PDT
When you change Color data layout please also remember to fix Tools/lldb/lldb_webkit.py