Extended Color Cleanup: Remove red()/green()/blue() accessors from ExtendedColor...
authorweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 May 2020 17:56:34 +0000 (17:56 +0000)
committerweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 May 2020 17:56:34 +0000 (17:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212366

Reviewed by Simon Fraser.

Source/WebCore:

* platform/graphics/Color.cpp:
(WebCore::differenceSquared):
Switch to using toSRGBASimpleColorLossy() rather than poking at the ExtendedColor
components directly. The old code was already incorrect if the two colors had
differing color spaces so this at least now gives reasonable results.

(WebCore::Color::colorWithAlpha const):
(WebCore::Color::colorWithAlphaUsingAlternativeRounding const):
Delegate to ExtendedColor completely for colorWithAlpha() to allow for more
control over how the components might be stored in the future.

* platform/graphics/Color.h:
(WebCore::Color::isBlackColor):
(WebCore::Color::isWhiteColor):
Delegate to ExtendedColor for isBlack()/isWhite() to allow per-color space
interpretations of the predicate.

(WebCore::Color::encode const):
(WebCore::Color::decode):
Use c1, c2, c3 rather than red/green/blue. This should be acceptable for the
forseeable future, as all expected colorspaces only have 3 channels, but at
some point, it may make sense to delegate coding to ExtendedColor completely.

* platform/graphics/ExtendedColor.cpp:
* platform/graphics/ExtendedColor.h:
(WebCore::ExtendedColor::channels const):
(WebCore::ExtendedColor::red const): Deleted.
(WebCore::ExtendedColor::green const): Deleted.
(WebCore::ExtendedColor::blue const): Deleted.
(WebCore::ExtendedColor::colorWithAlpha const): Added.
(WebCore::ExtendedColor::isWhite const): Added.
(WebCore::ExtendedColor::isBlack const): Added.
Use channels() with structured bindings rather than the red()/green()/blue() accessors
everywhere.

Tools:

* TestWebKitAPI/Tests/WebCore/ExtendedColorTests.cpp:
(TestWebKitAPI::TEST):
Update tests to pull values from the channels rather than from the red()/green()/blue() accessors.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@262151 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/Color.cpp
Source/WebCore/platform/graphics/Color.h
Source/WebCore/platform/graphics/ExtendedColor.cpp
Source/WebCore/platform/graphics/ExtendedColor.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebCore/ExtendedColorTests.cpp

index 2f12155..438719d 100644 (file)
@@ -1,3 +1,45 @@
+2020-05-26  Sam Weinig  <weinig@apple.com>
+
+        Extended Color Cleanup: Remove red()/green()/blue() accessors from ExtendedColor in preperation for supporting non-RGB based ColorSpaces
+        https://bugs.webkit.org/show_bug.cgi?id=212366
+
+        Reviewed by Simon Fraser.
+
+        * platform/graphics/Color.cpp:
+        (WebCore::differenceSquared):
+        Switch to using toSRGBASimpleColorLossy() rather than poking at the ExtendedColor
+        components directly. The old code was already incorrect if the two colors had 
+        differing color spaces so this at least now gives reasonable results.
+
+        (WebCore::Color::colorWithAlpha const):
+        (WebCore::Color::colorWithAlphaUsingAlternativeRounding const):
+        Delegate to ExtendedColor completely for colorWithAlpha() to allow for more
+        control over how the components might be stored in the future.
+        
+        * platform/graphics/Color.h:
+        (WebCore::Color::isBlackColor):
+        (WebCore::Color::isWhiteColor):
+        Delegate to ExtendedColor for isBlack()/isWhite() to allow per-color space
+        interpretations of the predicate.
+
+        (WebCore::Color::encode const):
+        (WebCore::Color::decode):
+        Use c1, c2, c3 rather than red/green/blue. This should be acceptable for the
+        forseeable future, as all expected colorspaces only have 3 channels, but at 
+        some point, it may make sense to delegate coding to ExtendedColor completely.
+
+        * platform/graphics/ExtendedColor.cpp:
+        * platform/graphics/ExtendedColor.h:
+        (WebCore::ExtendedColor::channels const):
+        (WebCore::ExtendedColor::red const): Deleted.
+        (WebCore::ExtendedColor::green const): Deleted.
+        (WebCore::ExtendedColor::blue const): Deleted.
+        (WebCore::ExtendedColor::colorWithAlpha const): Added.
+        (WebCore::ExtendedColor::isWhite const): Added.
+        (WebCore::ExtendedColor::isBlack const): Added.
+        Use channels() with structured bindings rather than the red()/green()/blue() accessors
+        everywhere.
+
 2020-05-26  Peng Liu  <peng.liu6@apple.com>
 
         ASSERTION FAILED: m_clientCounts.contains(contextId) - WebKit::VideoFullscreenManagerProxy::removeClientForContext()
index 0d16665..88c46c3 100644 (file)
@@ -41,16 +41,14 @@ static constexpr SimpleColor darkenedWhite { 0xFFABABAB };
 
 int differenceSquared(const Color& c1, const Color& c2)
 {
-    // FIXME: This is assuming that the colors are in the same colorspace.
+    // FIXME: ExtendedColor - needs to handle color spaces.
     // 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 :)
-    int c1Red = c1.isExtended() ? c1.asExtended().red() * 255 : c1.asSimpleColor().redComponent();
-    int c1Green = c1.isExtended() ? c1.asExtended().green() * 255 : c1.asSimpleColor().greenComponent();
-    int c1Blue = c1.isExtended() ? c1.asExtended().blue() * 255 : c1.asSimpleColor().blueComponent();
-    int c2Red = c2.isExtended() ? c2.asExtended().red() * 255 : c2.asSimpleColor().redComponent();
-    int c2Green = c2.isExtended() ? c2.asExtended().green() * 255 : c2.asSimpleColor().greenComponent();
-    int c2Blue = c2.isExtended() ? c2.asExtended().blue() * 255 : c2.asSimpleColor().blueComponent();
+
+    auto [c1Red, c1Blue, c1Green, c1Alpha] = c1.toSRGBASimpleColorLossy();
+    auto [c2Red, c2Blue, c2Green, c2Alpha] = c2.toSRGBASimpleColorLossy();
+
     int dR = c1Red - c2Red;
     int dG = c1Green - c2Green;
     int dB = c1Blue - c2Blue;
@@ -309,10 +307,8 @@ Color Color::colorWithAlphaMultipliedByUsingAlternativeRounding(float amount) co
 
 Color Color::colorWithAlpha(float alpha) const
 {
-    if (isExtended()) {
-        auto& extendedColor = asExtended();
-        return Color { extendedColor.red(), extendedColor.green(), extendedColor.blue(), alpha, extendedColor.colorSpace() };
-    }
+    if (isExtended())
+        return asExtended().colorWithAlpha(alpha);
 
     // FIXME: This is where this function differs from colorWithAlphaUsingAlternativeRounding.
     uint8_t newAlpha = alpha * 0xFF;
@@ -325,10 +321,8 @@ Color Color::colorWithAlpha(float alpha) const
 
 Color Color::colorWithAlphaUsingAlternativeRounding(float alpha) const
 {
-    if (isExtended()) {
-        auto& extendedColor = asExtended();
-        return Color { extendedColor.red(), extendedColor.green(), extendedColor.blue(), alpha, extendedColor.colorSpace() };
-    }
+    if (isExtended())
+        return asExtended().colorWithAlpha(alpha);
 
     // FIXME: This is where this function differs from colorWithAlphaUsing.
     uint8_t newAlpha = colorFloatToSimpleColorByte(alpha);
index 996d95c..ff20ff1 100644 (file)
@@ -337,21 +337,15 @@ inline void Color::setSimpleColor(SimpleColor simpleColor)
 
 inline bool Color::isBlackColor(const Color& color)
 {
-    if (color.isExtended()) {
-        const ExtendedColor& extendedColor = color.asExtended();
-        return !extendedColor.red() && !extendedColor.green() && !extendedColor.blue() && extendedColor.alpha() == 1;
-    }
-
+    if (color.isExtended())
+        return color.asExtended().isBlack();
     return color.asSimpleColor() == Color::black;
 }
 
 inline bool Color::isWhiteColor(const Color& color)
 {
-    if (color.isExtended()) {
-        const ExtendedColor& extendedColor = color.asExtended();
-        return extendedColor.red() == 1 && extendedColor.green() == 1 && extendedColor.blue() == 1 && extendedColor.alpha() == 1;
-    }
-    
+    if (color.isExtended())
+        return color.asExtended().isWhite();
     return color.asSimpleColor() == Color::white;
 }
 
@@ -360,11 +354,14 @@ void Color::encode(Encoder& encoder) const
 {
     if (isExtended()) {
         encoder << true;
-        encoder << asExtended().red();
-        encoder << asExtended().green();
-        encoder << asExtended().blue();
-        encoder << asExtended().alpha();
-        encoder << asExtended().colorSpace();
+
+        auto& extendedColor = asExtended();
+        auto [c1, c2, c3, alpha] = extendedColor.channels();
+        encoder << c1;
+        encoder << c2;
+        encoder << c3;
+        encoder << alpha;
+        encoder << extendedColor.colorSpace();
         return;
     }
 
@@ -389,22 +386,22 @@ Optional<Color> Color::decode(Decoder& decoder)
         return WTF::nullopt;
 
     if (isExtended) {
-        float red;
-        float green;
-        float blue;
+        float c1;
+        float c2;
+        float c3;
         float alpha;
         ColorSpace colorSpace;
-        if (!decoder.decode(red))
+        if (!decoder.decode(c1))
             return WTF::nullopt;
-        if (!decoder.decode(green))
+        if (!decoder.decode(c2))
             return WTF::nullopt;
-        if (!decoder.decode(blue))
+        if (!decoder.decode(c3))
             return WTF::nullopt;
         if (!decoder.decode(alpha))
             return WTF::nullopt;
         if (!decoder.decode(colorSpace))
             return WTF::nullopt;
-        return Color { red, green, blue, alpha, colorSpace };
+        return Color { c1, c2, c3, alpha, colorSpace };
     }
 
     bool isValid;
index ca768bf..44294d4 100644 (file)
@@ -39,7 +39,8 @@ Ref<ExtendedColor> ExtendedColor::create(float c1, float c2, float c3, float alp
 
 unsigned ExtendedColor::hash() const
 {
-    return computeHash(m_channels.components[0], m_channels.components[1], m_channels.components[2], m_channels.components[3], m_colorSpace);
+    auto [c1, c2, c3, alpha] = channels();
+    return computeHash(c1, c2, c3, alpha, m_colorSpace);
 }
 
 String ExtendedColor::cssText() const
@@ -57,16 +58,36 @@ String ExtendedColor::cssText() const
         return WTF::emptyString();
     }
 
+    auto [c1, c2, c3, existingAlpha] = channels();
+
     if (WTF::areEssentiallyEqual(alpha(), 1.0f))
-        return makeString("color(", colorSpace, ' ', red(), ' ', green(), ' ', blue(), ')');
+        return makeString("color(", colorSpace, ' ', c1, ' ', c2, ' ', c3, ')');
+
+    return makeString("color(", colorSpace, ' ', c1, ' ', c2, ' ', c3, " / ", existingAlpha, ')');
+}
 
-    return makeString("color(", colorSpace, ' ', red(), ' ', green(), ' ', blue(), " / ", alpha(), ')');
+Ref<ExtendedColor> ExtendedColor::colorWithAlpha(float overrideAlpha) const
+{
+    auto [c1, c2, c3, existingAlpha] = channels();
+    return ExtendedColor::create(c1, c2, c3, overrideAlpha, colorSpace());
 }
 
-Ref<ExtendedColor> ExtendedColor::invertedColorWithAlpha(float alpha) const
+Ref<ExtendedColor> ExtendedColor::invertedColorWithAlpha(float overrideAlpha) const
 {
     auto [c1, c2, c3, existingAlpha] = channels();
-    return ExtendedColor::create(1.0f - c1, 1.0f - c2, 1.0f - c3, alpha, colorSpace());
+    return ExtendedColor::create(1.0f - c1, 1.0f - c2, 1.0f - c3, overrideAlpha, colorSpace());
+}
+
+bool ExtendedColor::isWhite() const
+{
+    auto [c1, c2, c3, alpha] = channels();
+    return c1 == 1 && c2 == 1 && c3 == 1 && alpha == 1;
+}
+
+bool ExtendedColor::isBlack() const
+{
+    auto [c1, c2, c3, alpha] = channels();
+    return !c1 && !c2 && !c3 && alpha == 1;
 }
 
 }
index a04b450..a7a93fc 100644 (file)
@@ -26,7 +26,6 @@
 #pragma once
 
 #include "ColorSpace.h"
-
 #include "ColorUtilities.h"
 #include <wtf/Ref.h>
 #include <wtf/RefCounted.h>
@@ -38,33 +37,21 @@ class ExtendedColor : public RefCounted<ExtendedColor> {
 public:
     static Ref<ExtendedColor> create(float, float, float, float alpha, ColorSpace = ColorSpace::SRGB);
 
-    float red() const
-    {
-        return m_channels.components[0];
-    }
-
-    float green() const
-    {
-        return m_channels.components[1];
-    }
-
-    float blue() const
-    {
-        return m_channels.components[2];
-    }
-
     float alpha() const { return m_channels.components[3]; }
 
     const FloatComponents& channels() const { return m_channels; }
-
     ColorSpace colorSpace() const { return m_colorSpace; }
 
     WEBCORE_EXPORT unsigned hash() const;
 
     WEBCORE_EXPORT String cssText() const;
 
+    Ref<ExtendedColor> colorWithAlpha(float) const;
     Ref<ExtendedColor> invertedColorWithAlpha(float) const;
 
+    bool isWhite() const;
+    bool isBlack() const;
+
 private:
     ExtendedColor(float c1, float c2, float c3, float alpha, ColorSpace colorSpace)
         : m_channels(c1, c2, c3, alpha)
index c6a6877..ae3712f 100644 (file)
@@ -1,3 +1,14 @@
+2020-05-26  Sam Weinig  <weinig@apple.com>
+
+        Extended Color Cleanup: Remove red()/green()/blue() accessors from ExtendedColor in preperation for supporting non-RGB based ColorSpaces
+        https://bugs.webkit.org/show_bug.cgi?id=212366
+
+        Reviewed by Simon Fraser.
+
+        * TestWebKitAPI/Tests/WebCore/ExtendedColorTests.cpp:
+        (TestWebKitAPI::TEST):
+        Update tests to pull values from the channels rather than from the red()/green()/blue() accessors.
+
 2020-05-26  Peng Liu  <peng.liu6@apple.com>
 
         ASSERTION FAILED: m_clientCounts.contains(contextId) - WebKit::VideoFullscreenManagerProxy::removeClientForContext()
index fcf54fe..03cbfa8 100644 (file)
@@ -38,10 +38,13 @@ TEST(ExtendedColor, Constructor)
 {
     Color c1(1.0, 0.5, 0.25, 1.0, ColorSpace::DisplayP3);
     EXPECT_TRUE(c1.isExtended());
-    EXPECT_FLOAT_EQ(1.0, c1.asExtended().red());
-    EXPECT_FLOAT_EQ(0.5, c1.asExtended().green());
-    EXPECT_FLOAT_EQ(0.25, c1.asExtended().blue());
-    EXPECT_FLOAT_EQ(1.0, c1.asExtended().alpha());
+
+    auto [r, g, b, alpha] = c1.asExtended().channels();
+
+    EXPECT_FLOAT_EQ(1.0, r);
+    EXPECT_FLOAT_EQ(0.5, g);
+    EXPECT_FLOAT_EQ(0.25, b);
+    EXPECT_FLOAT_EQ(1.0, alpha);
     EXPECT_EQ(1u, c1.asExtended().refCount());
     EXPECT_EQ(c1.cssText(), "color(display-p3 1 0.5 0.25)");
 }
@@ -53,10 +56,12 @@ TEST(ExtendedColor, CopyConstructor)
 
     Color c2(c1);
 
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().red());
-    EXPECT_FLOAT_EQ(0.5, c2.asExtended().green());
-    EXPECT_FLOAT_EQ(0.25, c2.asExtended().blue());
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().alpha());
+    auto [r, g, b, alpha] = c2.asExtended().channels();
+
+    EXPECT_FLOAT_EQ(1.0, r);
+    EXPECT_FLOAT_EQ(0.5, g);
+    EXPECT_FLOAT_EQ(0.25, b);
+    EXPECT_FLOAT_EQ(1.0, alpha);
     EXPECT_EQ(2u, c1.asExtended().refCount());
     EXPECT_EQ(2u, c2.asExtended().refCount());
     EXPECT_EQ(c2.cssText(), "color(display-p3 1 0.5 0.25)");
@@ -69,10 +74,12 @@ TEST(ExtendedColor, Assignment)
 
     Color c2 = c1;
 
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().red());
-    EXPECT_FLOAT_EQ(0.5, c2.asExtended().green());
-    EXPECT_FLOAT_EQ(0.25, c2.asExtended().blue());
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().alpha());
+    auto [r, g, b, alpha] = c2.asExtended().channels();
+
+    EXPECT_FLOAT_EQ(1.0, r);
+    EXPECT_FLOAT_EQ(0.5, g);
+    EXPECT_FLOAT_EQ(0.25, b);
+    EXPECT_FLOAT_EQ(1.0, alpha);
     EXPECT_EQ(2u, c1.asExtended().refCount());
     EXPECT_EQ(2u, c2.asExtended().refCount());
     EXPECT_EQ(c2.cssText(), "color(display-p3 1 0.5 0.25)");
@@ -152,10 +159,12 @@ TEST(ExtendedColor, MoveConstructor)
     EXPECT_FALSE(c1.isExtended());
     EXPECT_FALSE(c1.isValid());
 
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().red());
-    EXPECT_FLOAT_EQ(0.5, c2.asExtended().green());
-    EXPECT_FLOAT_EQ(0.25, c2.asExtended().blue());
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().alpha());
+    auto [r, g, b, alpha] = c2.asExtended().channels();
+
+    EXPECT_FLOAT_EQ(1.0, r);
+    EXPECT_FLOAT_EQ(0.5, g);
+    EXPECT_FLOAT_EQ(0.25, b);
+    EXPECT_FLOAT_EQ(1.0, alpha);
     EXPECT_EQ(1u, c2.asExtended().refCount());
     EXPECT_EQ(c2.cssText(), "color(display-p3 1 0.5 0.25)");
 }
@@ -172,10 +181,12 @@ TEST(ExtendedColor, MoveAssignment)
     EXPECT_FALSE(c1.isExtended());
     EXPECT_FALSE(c1.isValid());
 
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().red());
-    EXPECT_FLOAT_EQ(0.5, c2.asExtended().green());
-    EXPECT_FLOAT_EQ(0.25, c2.asExtended().blue());
-    EXPECT_FLOAT_EQ(1.0, c2.asExtended().alpha());
+    auto [r, g, b, alpha] = c2.asExtended().channels();
+
+    EXPECT_FLOAT_EQ(1.0, r);
+    EXPECT_FLOAT_EQ(0.5, g);
+    EXPECT_FLOAT_EQ(0.25, b);
+    EXPECT_FLOAT_EQ(1.0, alpha);
     EXPECT_EQ(1u, c2.asExtended().refCount());
     EXPECT_EQ(c2.cssText(), "color(display-p3 1 0.5 0.25)");
 }
@@ -188,10 +199,12 @@ TEST(ExtendedColor, BasicReferenceCounting)
     Color* c2 = new Color(*c1);
     Color* c3 = new Color(*c2);
 
-    EXPECT_FLOAT_EQ(1.0, c2->asExtended().red());
-    EXPECT_FLOAT_EQ(0.5, c2->asExtended().green());
-    EXPECT_FLOAT_EQ(0.25, c2->asExtended().blue());
-    EXPECT_FLOAT_EQ(1.0, c2->asExtended().alpha());
+    auto [r, g, b, alpha] = c2->asExtended().channels();
+
+    EXPECT_FLOAT_EQ(1.0, r);
+    EXPECT_FLOAT_EQ(0.5, g);
+    EXPECT_FLOAT_EQ(0.25, b);
+    EXPECT_FLOAT_EQ(1.0, alpha);
     EXPECT_EQ(3u, c2->asExtended().refCount());
     EXPECT_EQ(c2->cssText(), "color(display-p3 1 0.5 0.25)");