Bug 214522 – Rename Color::transparent to Color::transparentBlack to more clearly state what it is

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

[Sam Weinig]

Rename Color::transparent to Color::transparentBlack to more clearly state what it is

[Sam Weinig]

Created attachment 404645 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Darin Adler]

Comment on attachment 404645 [details]
Patch

Should we also consider using the named version less? I think that sometimes returning { } instead of Color::transparentBlack would represent our intent more clearly.

[Darin Adler]

Comment on attachment 404645 [details]
Patch

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

Just realized that for Color { } is "invalid transparent black" not "valid transparent black", which affects my earlier comment. For SRGBA<uint8_t> { } is "transparent black".

Not really sure about the many places we use transparent black but could be using invalid instead.

> Source/WebCore/accessibility/AccessibilityObject.h:406
> +    SRGBA<uint8_t> colorValue() const override { return Color::transparentBlack; }

Perfect example of a place where I am not sure explicitly returning transparentBlack is the right thing. Also, violates Liskov substitution principle, but so does so much of our code.

> Source/WebCore/editing/EditingStyle.cpp:394
> -        return Color::transparent;
> +        return Color::transparentBlack;

Interesting choice that we use "valid transparent black" for this, not "invalid transparent black".

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:134
> +        return Color::transparentBlack;

Truly peculiar choice to represent parsing failure as "valid transparent black".

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:140
> +        return Color::transparentBlack;

Ditto.

> Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:274
>      if (!windowColor.isValid())
> -        windowColor = Color::transparent;
> +        windowColor = Color::transparentBlack;

This is the "make valid" pattern (see below).

> Source/WebCore/page/TextIndicator.cpp:224
> +    auto estimatedBackgroundColor = frame.view() ? frame.view()->documentBackgroundColor() : Color::transparentBlack;

Interesting to use "valid" instead of "invalid" here.

> Source/WebCore/platform/graphics/Color.h:149
> +    static constexpr auto transparentBlack = ColorBuilder<SRGBA<uint8_t>> { };

As the list of colors gets longer we may want to consider just alphabetical sorting instead of logical sorting.

> Source/WebCore/rendering/style/RenderStyle.cpp:2063
> -    if (colorProperty == CSSPropertyBackgroundColor && visitedColor == Color::transparent)
> +    if (colorProperty == CSSPropertyBackgroundColor && visitedColor == Color::transparentBlack)
>          return unvisitedColor;

Should this be checking for any color with alpha 0? Why is specifically checking for transparent black a good idea.

> Source/WebCore/style/StyleBuilderCustom.h:836
> +        auto shadowData = makeUnique<ShadowData>(LayoutPoint(x, y), blur, spread, shadowStyle, property == CSSPropertyWebkitBoxShadow, color.isValid() ? color : Color::transparentBlack);

This is the "make valid" operation. Not sure why we ever need to do this, specifically why we need to do it here, and this should be a named function, not this peculiar-looking expression.

> Source/WebCore/style/StyleBuilderState.cpp:269
> +            operations.operations().append(DropShadowFilterOperation::create(location, blur, color.isValid() ? color : Color::transparentBlack));

Ditto.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.mm:95
> -    , backgroundColor(WebCore::Color::transparent)
> +    , backgroundColor(WebCore::Color::transparentBlack)

Seems like this is already the default and would happen without an initializer.

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 404645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=404645&action=review
> 
> Just realized that for Color { } is "invalid transparent black" not "valid
> transparent black", which affects my earlier comment. For SRGBA<uint8_t> { }
> is "transparent black".
> 
> Not really sure about the many places we use transparent black but could be
> using invalid instead.
> 
> > Source/WebCore/accessibility/AccessibilityObject.h:406
> > +    SRGBA<uint8_t> colorValue() const override { return Color::transparentBlack; }
> 
> Perfect example of a place where I am not sure explicitly returning
> transparentBlack is the right thing. Also, violates Liskov substitution
> principle, but so does so much of our code.

Liskov breaks down a bit, at least as I understand it, when you start talking about type conversions, which is what's going on here. SRGBA<uint8_t> does not expose anything like the API of Color on purpose, it's just meant to be the data. That said, your point is just as valid that the isValid() bit on Color is really quite problematic.

Wondering out loud without a clear opinion yet, I wonder if we made invalid Color objects not act like transparent black (e.g. make functions like toSRGBALossy() return Optional<SRGBA<T>>) if that would improve things or make things worse. I'm inclined to think it would make things work, and working towards getting rid of valid bit is probably a much better way forward. 

Because we would want to replace some uses of Color with Optional<Color>, in the few places where there is some need to differentiate, it leads me to another question I am not clear on. Should we ever specialize Optional<T> such that rather than being implemented as (roughly) Optional { T value; bool hasValue; } we instead just store T, and use a trick like HashTableEmptyValueType to store the invalid state in the object itself. This would be nice, since you could then use Optional<T> in places where you don't want to waste space. I have not done something like this so far as I am not sure (and pretty doubtful) if it would be compatible with std::optional<> if we ever wanted to move to it.

[Sam Weinig]

Created attachment 404681 [details]
Patch

[EWS]

Found 1 new test failure: webgl/2.0.0/conformance2/state/gl-object-get-calls.html

[Sam Weinig]

(In reply to EWS from comment #7)
> Found 1 new test failure:
> webgl/2.0.0/conformance2/state/gl-object-get-calls.html

I don't believe this is related, just a flakey test.

[Sam Weinig]

Committed r264585: <https://trac.webkit.org/changeset/264585>

[Radar WebKit Bug Importer]

<rdar://problem/65798718>