Remove almost always incorrect Color::getRGBA
authorweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 May 2020 02:13:57 +0000 (02:13 +0000)
committerweinig@apple.com <weinig@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 May 2020 02:13:57 +0000 (02:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=212059

Reviewed by Darin Adler and Simon Fraser.

Source/WebCore:

Removes the awkward and almost always incorrect out parameter based Color::getRGBA. Most existing callsites
were updated to use Color::toSRGBAComponentsLossy() or other more ExtendedColor supporting functions (like
cachedCGColor()).

Also adds tuple-like adaptors for FloatComponents to allow it to be used
with structured bindings. For example:

    auto [r, g, b, a] = myFloatComponents;

* platform/graphics/Color.h:
* platform/graphics/Color.cpp:
(WebCore::Color::light const):
(WebCore::Color::dark const):
(WebCore::Color::isDark const):
Adopt toSRGBAComponentsLossy() to make these not return totally incorrect values.

(WebCore::Color::colorSpaceAndComponents const):
Added. Returns a std::pair<ColorSpace, FloatComponents> for functions that need
to operate on the components in a ColorSpace aware way. Used by toSRGBAComponentsLossy
and leakCGColor() for now, but will be useful going forward as well.

(WebCore::Color::toSRGBAComponentsLossy const):
Re-implement using colorSpaceAndComponents() to simplify implementation.

(WebCore::Color::getRGBA const): Deleted.

* platform/graphics/ColorUtilities.cpp:
(WebCore::FloatComponents::FloatComponents): Deleted.
(WebCore::sRGBColorToLinearComponents): Deleted.
* platform/graphics/ColorUtilities.h:
(WebCore::FloatComponents::get const):
Remove uses of the Color class to simplify inlining (Color.h already needs to know about
FloatComponents).
    - FloatComponents constructor replaced by Color::toSRGBAComponentsLossy().
    - sRGBColorToLinearComponents replaced by calling rgbToLinearComponents(color.toSRGBAComponentsLossy()).

Also adds std::tuple adaptors for FloatComponents to allow using with stuctured bindings.

* page/cocoa/ResourceUsageOverlayCocoa.mm:
(WebCore::HistoricMemoryCategoryInfo::HistoricMemoryCategoryInfo):
Replace getRGBA with cachedCGColor.

* platform/graphics/ca/win/PlatformCALayerWin.cpp:
(PlatformCALayerWin::setBackgroundColor):
Replace getRGBA with cachedCGColor.

* platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:
(PlatformCALayerWinInternal::setBorderColor):
Replace getRGBA with cachedCGColor.

* platform/graphics/cairo/CairoUtilities.cpp:
(WebCore::setSourceRGBAFromColor):
Replace getRGBA with toSRGBAComponentsLossy.

* platform/graphics/cairo/GradientCairo.cpp:
(WebCore::addColorStopRGBA):
(WebCore::setCornerColorRGBA):
(WebCore::interpolateColorStop):
Replace getRGBA with toSRGBAComponentsLossy.

* platform/graphics/cg/ColorCG.cpp:
(WebCore::leakCGColor):
(WebCore::cachedCGColor):
Simplify implementation (and remove use of getRGBA) by using colorSpaceAndComponents().

* platform/graphics/cg/GradientCG.cpp:
(WebCore::Gradient::platformGradient):
Replace getRGBA with colorSpaceAndComponent().

* platform/graphics/filters/FELighting.cpp:
(WebCore::FELighting::drawLighting):
Replace FloatComponents constructor taking a Color (which used getRGBA) with toSRGBAComponentsLossy.

* platform/graphics/filters/FilterOperations.cpp:
(WebCore::FilterOperations::transformColor const):
(WebCore::FilterOperations::inverseTransformColor const):
Replace getRGBA with toSRGBAComponentsLossy.

* platform/graphics/gtk/ColorGtk.cpp:
(WebCore::Color::operator GdkRGBA const):
Replace getRGBA with toSRGBAComponentsLossy.

* platform/graphics/texmap/TextureMapperGL.cpp:
(WebCore::TextureMapperGL::drawBorder):
(WebCore::TextureMapperGL::drawNumber):
(WebCore::prepareFilterProgram):
(WebCore::TextureMapperGL::drawSolidColor):
Replace getRGBA with toSRGBAComponentsLossy.

* platform/graphics/win/GradientDirect2D.cpp:
(WebCore::Gradient::generateGradient):
Replace getRGBA with toSRGBAComponentsLossy.

* platform/graphics/win/GraphicsContextCGWin.cpp:
(WebCore::GraphicsContext::drawDotsForDocumentMarker):
(WebCore::setCGStrokeColor): Deleted.
(WebCore::spellingPatternColor): Deleted.
(WebCore::grammarPatternColor): Deleted.
Replace use of getRGBA with direct use of SimpleColor instead.

* rendering/TextPaintStyle.cpp:
(WebCore::textColorIsLegibleAgainstBackgroundColor):
Replace implicit FloatComponents construction taking a Color (which used getRGBA) with explicit toSRGBAComponentsLossy.

Source/WebKit:

* UIProcess/API/wpe/WebKitColor.cpp:
(webkitColorFillFromWebCoreColor):
* UIProcess/gtk/ViewGestureControllerGtk.cpp:
(WebKit::ViewGestureController::beginSwipeGesture):
* WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::getDocumentBackgroundColor):
Replace Color::getRGBA with Color::toSRGBAComponentsLossy()

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

23 files changed:
Source/WebCore/ChangeLog
Source/WebCore/page/cocoa/ResourceUsageOverlayCocoa.mm
Source/WebCore/platform/graphics/Color.cpp
Source/WebCore/platform/graphics/Color.h
Source/WebCore/platform/graphics/ColorUtilities.cpp
Source/WebCore/platform/graphics/ColorUtilities.h
Source/WebCore/platform/graphics/ca/win/PlatformCALayerWin.cpp
Source/WebCore/platform/graphics/ca/win/PlatformCALayerWinInternal.cpp
Source/WebCore/platform/graphics/cairo/CairoUtilities.cpp
Source/WebCore/platform/graphics/cairo/GradientCairo.cpp
Source/WebCore/platform/graphics/cg/ColorCG.cpp
Source/WebCore/platform/graphics/cg/GradientCG.cpp
Source/WebCore/platform/graphics/filters/FELighting.cpp
Source/WebCore/platform/graphics/filters/FilterOperations.cpp
Source/WebCore/platform/graphics/gtk/ColorGtk.cpp
Source/WebCore/platform/graphics/texmap/TextureMapperGL.cpp
Source/WebCore/platform/graphics/win/GradientDirect2D.cpp
Source/WebCore/platform/graphics/win/GraphicsContextCGWin.cpp
Source/WebCore/rendering/TextPaintStyle.cpp
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/API/wpe/WebKitColor.cpp
Source/WebKit/UIProcess/gtk/ViewGestureControllerGtk.cpp
Source/WebKit/WebProcess/WebPage/WebFrame.cpp

index 5a50a13..69ae067 100644 (file)
@@ -1,3 +1,114 @@
+2020-05-19  Sam Weinig  <weinig@apple.com>
+
+        Remove almost always incorrect Color::getRGBA
+        https://bugs.webkit.org/show_bug.cgi?id=212059
+
+        Reviewed by Darin Adler and Simon Fraser.
+
+        Removes the awkward and almost always incorrect out parameter based Color::getRGBA. Most existing callsites
+        were updated to use Color::toSRGBAComponentsLossy() or other more ExtendedColor supporting functions (like 
+        cachedCGColor()). 
+        
+        Also adds tuple-like adaptors for FloatComponents to allow it to be used
+        with structured bindings. For example:
+        
+            auto [r, g, b, a] = myFloatComponents;
+
+        * platform/graphics/Color.h:
+        * platform/graphics/Color.cpp:
+        (WebCore::Color::light const):
+        (WebCore::Color::dark const):
+        (WebCore::Color::isDark const):
+        Adopt toSRGBAComponentsLossy() to make these not return totally incorrect values.
+
+        (WebCore::Color::colorSpaceAndComponents const):
+        Added. Returns a std::pair<ColorSpace, FloatComponents> for functions that need
+        to operate on the components in a ColorSpace aware way. Used by toSRGBAComponentsLossy
+        and leakCGColor() for now, but will be useful going forward as well.
+
+        (WebCore::Color::toSRGBAComponentsLossy const):
+        Re-implement using colorSpaceAndComponents() to simplify implementation.
+
+        (WebCore::Color::getRGBA const): Deleted.
+        
+        * platform/graphics/ColorUtilities.cpp:
+        (WebCore::FloatComponents::FloatComponents): Deleted.
+        (WebCore::sRGBColorToLinearComponents): Deleted.
+        * platform/graphics/ColorUtilities.h:
+        (WebCore::FloatComponents::get const):
+        Remove uses of the Color class to simplify inlining (Color.h already needs to know about
+        FloatComponents). 
+            - FloatComponents constructor replaced by Color::toSRGBAComponentsLossy(). 
+            - sRGBColorToLinearComponents replaced by calling rgbToLinearComponents(color.toSRGBAComponentsLossy()).
+        
+        Also adds std::tuple adaptors for FloatComponents to allow using with stuctured bindings. 
+
+        * page/cocoa/ResourceUsageOverlayCocoa.mm:
+        (WebCore::HistoricMemoryCategoryInfo::HistoricMemoryCategoryInfo):
+        Replace getRGBA with cachedCGColor.
+        
+        * platform/graphics/ca/win/PlatformCALayerWin.cpp:
+        (PlatformCALayerWin::setBackgroundColor):
+        Replace getRGBA with cachedCGColor.
+
+        * platform/graphics/ca/win/PlatformCALayerWinInternal.cpp:
+        (PlatformCALayerWinInternal::setBorderColor):
+        Replace getRGBA with cachedCGColor.
+
+        * platform/graphics/cairo/CairoUtilities.cpp:
+        (WebCore::setSourceRGBAFromColor):
+        Replace getRGBA with toSRGBAComponentsLossy.
+
+        * platform/graphics/cairo/GradientCairo.cpp:
+        (WebCore::addColorStopRGBA):
+        (WebCore::setCornerColorRGBA):
+        (WebCore::interpolateColorStop):
+        Replace getRGBA with toSRGBAComponentsLossy.
+
+        * platform/graphics/cg/ColorCG.cpp:
+        (WebCore::leakCGColor):
+        (WebCore::cachedCGColor):
+        Simplify implementation (and remove use of getRGBA) by using colorSpaceAndComponents().
+
+        * platform/graphics/cg/GradientCG.cpp:
+        (WebCore::Gradient::platformGradient):
+        Replace getRGBA with colorSpaceAndComponent(). 
+        
+        * platform/graphics/filters/FELighting.cpp:
+        (WebCore::FELighting::drawLighting):
+        Replace FloatComponents constructor taking a Color (which used getRGBA) with toSRGBAComponentsLossy.
+    
+        * platform/graphics/filters/FilterOperations.cpp:
+        (WebCore::FilterOperations::transformColor const):
+        (WebCore::FilterOperations::inverseTransformColor const):
+        Replace getRGBA with toSRGBAComponentsLossy.
+
+        * platform/graphics/gtk/ColorGtk.cpp:
+        (WebCore::Color::operator GdkRGBA const):
+        Replace getRGBA with toSRGBAComponentsLossy.
+
+        * platform/graphics/texmap/TextureMapperGL.cpp:
+        (WebCore::TextureMapperGL::drawBorder):
+        (WebCore::TextureMapperGL::drawNumber):
+        (WebCore::prepareFilterProgram):
+        (WebCore::TextureMapperGL::drawSolidColor):
+        Replace getRGBA with toSRGBAComponentsLossy.
+
+        * platform/graphics/win/GradientDirect2D.cpp:
+        (WebCore::Gradient::generateGradient):
+        Replace getRGBA with toSRGBAComponentsLossy.
+
+        * platform/graphics/win/GraphicsContextCGWin.cpp:
+        (WebCore::GraphicsContext::drawDotsForDocumentMarker):
+        (WebCore::setCGStrokeColor): Deleted.
+        (WebCore::spellingPatternColor): Deleted.
+        (WebCore::grammarPatternColor): Deleted.
+        Replace use of getRGBA with direct use of SimpleColor instead.
+        
+        * rendering/TextPaintStyle.cpp:
+        (WebCore::textColorIsLegibleAgainstBackgroundColor):
+        Replace implicit FloatComponents construction taking a Color (which used getRGBA) with explicit toSRGBAComponentsLossy.
+
 2020-05-19  Eric Carlson  <eric.carlson@apple.com>
 
         Update some media logging
index c6e1872..e263934 100644 (file)
@@ -133,9 +133,7 @@ struct HistoricMemoryCategoryInfo {
         , isSubcategory(subcategory)
         , type(category)
     {
-        float r, g, b, a;
-        Color { SimpleColor { argb } }.getRGBA(r, g, b, a);
-        color = adoptCF(createColor(r, g, b, a));
+        color = cachedCGColor(SimpleColor { argb });
     }
 
     String name;
index d3d8164..d1087b1 100644 (file)
@@ -350,14 +350,14 @@ Color Color::light() const
     
     const float scaleFactor = nextafterf(256.0f, 0.0f);
 
-    float r, g, b, a;
-    getRGBA(r, g, b, a);
+    auto [r, g, b, a] = toSRGBAComponentsLossy();
 
-    float v = std::max(r, std::max(g, b));
+    float v = std::max({ r, g, b });
 
-    if (v == 0.0f)
+    if (v == 0.0f) {
         // Lightened black with alpha.
         return Color(0x54, 0x54, 0x54, alpha());
+    }
 
     float multiplier = std::min(1.0f, v + 0.33f) / v;
 
@@ -375,10 +375,9 @@ Color Color::dark() const
     
     const float scaleFactor = nextafterf(256.0f, 0.0f);
 
-    float r, g, b, a;
-    getRGBA(r, g, b, a);
+    auto [r, g, b, a] = toSRGBAComponentsLossy();
 
-    float v = std::max(r, std::max(g, b));
+    float v = std::max({ r, g, b });
     float multiplier = std::max(0.0f, (v - 0.33f) / v);
 
     return Color(static_cast<int>(multiplier * r * scaleFactor),
@@ -389,13 +388,10 @@ Color Color::dark() const
 
 bool Color::isDark() const
 {
-    float red;
-    float green;
-    float blue;
-    float alpha;
-    getRGBA(red, green, blue, alpha);
-    float largestNonAlphaChannel = std::max(red, std::max(green, blue));
-    return alpha > 0.5 && largestNonAlphaChannel < 0.5;
+    // FIXME: This should probably be using luminance.
+    auto [r, g, b, a] = toSRGBAComponentsLossy();
+    float largestNonAlphaChannel = std::max({ r, g, b });
+    return a > 0.5 && largestNonAlphaChannel < 0.5;
 }
 
 static int blendComponent(int c, int a)
@@ -471,22 +467,6 @@ Color Color::colorWithAlpha(float alpha) const
     return result;
 }
 
-void Color::getRGBA(float& r, float& g, float& b, float& a) const
-{
-    r = red() / 255.0f;
-    g = green() / 255.0f;
-    b = blue() / 255.0f;
-    a = alpha() / 255.0f;
-}
-
-void Color::getRGBA(double& r, double& g, double& b, double& a) const
-{
-    r = red() / 255.0;
-    g = green() / 255.0;
-    b = blue() / 255.0;
-    a = alpha() / 255.0;
-}
-
 // FIXME: Use sRGBToHSL().
 void Color::getHSL(double& hue, double& saturation, double& lightness) const
 {
@@ -555,22 +535,29 @@ void Color::getHSV(double& hue, double& saturation, double& value) const
     value = max;
 }
 
-FloatComponents Color::toSRGBAComponentsLossy() const
+std::pair<ColorSpace, FloatComponents> Color::colorSpaceAndComponents() const
 {
     if (isExtended()) {
         auto& extendedColor = asExtended();
-        switch (extendedColor.colorSpace()) {
-        case ColorSpace::SRGB:
-            return extendedColor.channels();
-        case ColorSpace::LinearRGB:
-            return linearToRGBComponents(extendedColor.channels());
-        case ColorSpace::DisplayP3:
-            return p3ToSRGB(extendedColor.channels());
-        }
+        return { extendedColor.colorSpace(), extendedColor.channels() };
+    }
+
+    return { ColorSpace::SRGB, FloatComponents { red() / 255.0f, green() / 255.0f, blue() / 255.0f,  alpha() / 255.0f } };
+}
+
+FloatComponents Color::toSRGBAComponentsLossy() const
+{
+    auto [colorSpace, components] = colorSpaceAndComponents();
+    switch (colorSpace) {
+    case ColorSpace::SRGB:
+        return components;
+    case ColorSpace::LinearRGB:
+        return linearToRGBComponents(components);
+    case ColorSpace::DisplayP3:
+        return p3ToSRGB(components);
     }
-    float r, g, b, a;
-    getRGBA(r, g, b, a);
-    return { r, g, b, a };
+    ASSERT_NOT_REACHED();
+    return { };
 }
 
 Color colorFromPremultipliedARGB(RGBA32 pixelColor)
index 556e95f..c29eece 100644 (file)
@@ -209,12 +209,12 @@ public:
 
     // FIXME: ExtendedColor - these should be renamed (to be clear about their parameter types, or
     // replaced with alternative accessors.
-    WEBCORE_EXPORT void getRGBA(float& r, float& g, float& b, float& a) const;
-    WEBCORE_EXPORT void getRGBA(double& r, double& g, double& b, double& a) const;
 
     WEBCORE_EXPORT void getHSL(double& h, double& s, double& l) const;
     WEBCORE_EXPORT void getHSV(double& h, double& s, double& v) const;
 
+    WEBCORE_EXPORT std::pair<ColorSpace, FloatComponents> colorSpaceAndComponents() const;
+
     // This will convert non-sRGB colorspace colors into sRGB.
     WEBCORE_EXPORT FloatComponents toSRGBAComponentsLossy() const;
 
index e20a058..abdd021 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017, 2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
 #include "config.h"
 #include "ColorUtilities.h"
 
-#include "Color.h"
 #include <wtf/MathExtras.h>
 
 namespace WebCore {
 
-FloatComponents::FloatComponents(const Color& color)
-{
-    color.getRGBA(components[0], components[1], components[2], components[3]);
-}
-
 ColorComponents::ColorComponents(const FloatComponents& floatComponents)
 {
     components[0] = clampedColorComponent(floatComponents.components[0]);
@@ -69,18 +63,6 @@ float rgbToLinearColorComponent(float c)
     return clampTo<float>(std::pow((c + 0.055f) / 1.055f, 2.4f), 0, 1);
 }
 
-FloatComponents sRGBColorToLinearComponents(const Color& color)
-{
-    float r, g, b, a;
-    color.getRGBA(r, g, b, a);
-    return {
-        rgbToLinearColorComponent(r),
-        rgbToLinearColorComponent(g),
-        rgbToLinearColorComponent(b),
-        a
-    };
-}
-
 FloatComponents rgbToLinearComponents(const FloatComponents& RGBColor)
 {
     return {
index d62a60e..fa5a745 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2017, 2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -31,8 +31,6 @@
 
 namespace WebCore {
 
-class Color;
-
 struct FloatComponents {
     FloatComponents(float a = 0, float b = 0, float c = 0, float d = 0)
     {
@@ -50,8 +48,6 @@ struct FloatComponents {
         components[3] = values[3];
     }
 
-    FloatComponents(const Color&);
-
     FloatComponents& operator+=(const FloatComponents& rhs)
     {
         components[0] += rhs.components[0];
@@ -101,6 +97,12 @@ struct FloatComponents {
         return result;
     }
 
+    template<std::size_t N>
+    float get() const
+    {
+        return components[N];
+    }
+
     std::array<float, 4> components;
 };
 
@@ -176,7 +178,6 @@ inline unsigned byteOffsetOfPixel(unsigned x, unsigned y, unsigned rowBytes)
 float linearToRGBColorComponent(float);
 float rgbToLinearColorComponent(float);
 
-FloatComponents sRGBColorToLinearComponents(const Color&);
 FloatComponents rgbToLinearComponents(const FloatComponents&);
 FloatComponents linearToRGBComponents(const FloatComponents&);
 
@@ -210,3 +211,16 @@ private:
 
 } // namespace WebCore
 
+namespace std {
+
+template<>
+class tuple_size<WebCore::FloatComponents> : public std::integral_constant<std::size_t, 4> {
+};
+
+template<std::size_t N>
+class tuple_element<N, WebCore::FloatComponents> {
+public:
+    using type = float;
+};
+
+}
index 7a652f1..e195790 100644 (file)
@@ -576,13 +576,7 @@ Color PlatformCALayerWin::backgroundColor() const
 
 void PlatformCALayerWin::setBackgroundColor(const Color& value)
 {
-    CGFloat components[4];
-    value.getRGBA(components[0], components[1], components[2], components[3]);
-
-    RetainPtr<CGColorSpaceRef> colorSpace = adoptCF(CGColorSpaceCreateDeviceRGB());
-    RetainPtr<CGColorRef> color = adoptCF(CGColorCreate(colorSpace.get(), components));
-
-    CACFLayerSetBackgroundColor(m_layer.get(), color.get());
+    CACFLayerSetBackgroundColor(m_layer.get(), cachedCGColor(value));
     setNeedsCommit();
 }
 
index 7bdbeeb..7ed95a2 100644 (file)
@@ -312,15 +312,7 @@ void PlatformCALayerWinInternal::setBorderWidth(float value)
 
 void PlatformCALayerWinInternal::setBorderColor(const Color& value)
 {
-    CGFloat components[4] = { 0, 0, 0, 0 };
-    RetainPtr<CGColorSpaceRef> colorSpace = adoptCF(CGColorSpaceCreateDeviceRGB());
-
-    if (value.isValid())
-        value.getRGBA(components[0], components[1], components[2], components[3]);
-
-    RetainPtr<CGColorRef> color = adoptCF(CGColorCreate(colorSpace.get(), components));
-
-    CACFLayerSetBorderColor(owner()->platformLayer(), color.get());
+    CACFLayerSetBorderColor(owner()->platformLayer(), cachedCGColor(value));
 }
 
 #endif
index 9c197ed..34d935b 100644 (file)
@@ -83,13 +83,8 @@ void copyContextProperties(cairo_t* srcCr, cairo_t* dstCr)
 
 void setSourceRGBAFromColor(cairo_t* context, const Color& color)
 {
-    if (color.isExtended())
-        cairo_set_source_rgba(context, color.asExtended().red(), color.asExtended().green(), color.asExtended().blue(), color.asExtended().alpha());
-    else {
-        float red, green, blue, alpha;
-        color.getRGBA(red, green, blue, alpha);
-        cairo_set_source_rgba(context, red, green, blue, alpha);
-    }
+    auto [r, g, b, a] = color.toSRGBAComponentsLossy();
+    cairo_set_source_rgba(context, r, g, b, a);
 }
 
 void appendPathToCairoContext(cairo_t* to, cairo_t* from)
index dce37c6..d5aeabc 100644 (file)
@@ -43,14 +43,8 @@ void Gradient::platformDestroy()
 
 static void addColorStopRGBA(cairo_pattern_t *gradient, Gradient::ColorStop stop, float globalAlpha)
 {
-    if (stop.color.isExtended()) {
-        cairo_pattern_add_color_stop_rgba(gradient, stop.offset, stop.color.asExtended().red(), stop.color.asExtended().green(),
-            stop.color.asExtended().blue(), stop.color.asExtended().alpha() * globalAlpha);
-    } else {
-        float r, g, b, a;
-        stop.color.getRGBA(r, g, b, a);
-        cairo_pattern_add_color_stop_rgba(gradient, stop.offset, r, g, b, a * globalAlpha);
-    }
+    auto [r, g, b, a] = stop.color.toSRGBAComponentsLossy();
+    cairo_pattern_add_color_stop_rgba(gradient, stop.offset, r, g, b, a * globalAlpha);
 }
 
 #if PLATFORM(GTK) || PLATFORM(WPE)
@@ -61,14 +55,8 @@ typedef struct point_t {
 
 static void setCornerColorRGBA(cairo_pattern_t* gradient, int id, Gradient::ColorStop stop, float globalAlpha)
 {
-    if (stop.color.isExtended()) {
-        cairo_mesh_pattern_set_corner_color_rgba(gradient, id, stop.color.asExtended().red(), stop.color.asExtended().green(),
-            stop.color.asExtended().blue(), stop.color.asExtended().alpha() * globalAlpha);
-    } else {
-        float r, g, b, a;
-        stop.color.getRGBA(r, g, b, a);
-        cairo_mesh_pattern_set_corner_color_rgba(gradient, id, r, g, b, a * globalAlpha);
-    }
+    auto [r, g, b, a] = stop.color.toSRGBAComponentsLossy();
+    cairo_mesh_pattern_set_corner_color_rgba(gradient, id, r, g, b, a * globalAlpha);
 }
 
 static void addConicSector(cairo_pattern_t *gradient, float cx, float cy, float r, float angleRadians,
@@ -147,24 +135,8 @@ static void addConicSector(cairo_pattern_t *gradient, float cx, float cy, float
 
 static Gradient::ColorStop interpolateColorStop(Gradient::ColorStop from, Gradient::ColorStop to)
 {
-    float r1, g1, b1, a1;
-    float r2, g2, b2, a2;
-
-    if (from.color.isExtended()) {
-        r1 = from.color.asExtended().red();
-        g1 = from.color.asExtended().green();
-        b1 = from.color.asExtended().blue();
-        a1 = from.color.asExtended().alpha();
-    } else
-        from.color.getRGBA(r1, g1, b1, a1);
-
-    if (to.color.isExtended()) {
-        r2 = to.color.asExtended().red();
-        g2 = to.color.asExtended().green();
-        b2 = to.color.asExtended().blue();
-        a2 = to.color.asExtended().alpha();
-    } else
-        to.color.getRGBA(r2, g2, b2, a2);
+    auto [r1, g1, b1, a1] = from.color.toSRGBAComponentsLossy();
+    auto [r2, g2, b2, a2] = to.color.toSRGBAComponentsLossy();
 
     float offset = from.offset + (to.offset - from.offset) * 0.5f;
     float r = r1 + (r2 - r1) * 0.5f;
index d71ac62..ff38d4e 100644 (file)
@@ -103,27 +103,22 @@ Color::Color(CGColorRef color, SemanticTag)
 
 static CGColorRef leakCGColor(const Color& color)
 {
-    CGFloat components[4];
-    if (color.isExtended()) {
-        const auto& extendedColor = color.asExtended();
-        auto channels = extendedColor.channels();
-        components[0] = channels.components[0];
-        components[1] = channels.components[1];
-        components[2] = channels.components[2];
-        components[3] = channels.components[3];
-        switch (extendedColor.colorSpace()) {
-        case ColorSpace::SRGB:
-            return CGColorCreate(sRGBColorSpaceRef(), components);
-        case ColorSpace::DisplayP3:
-            return CGColorCreate(displayP3ColorSpaceRef(), components);
-        case ColorSpace::LinearRGB:
-            // FIXME: Do we ever create CGColorRefs in these spaces? It may only be ImageBuffers.
-            return CGColorCreate(sRGBColorSpaceRef(), components);
-        }
+    auto [colorSpace, components] = color.colorSpaceAndComponents();
+    auto [r, g, b, a] = components;
+    CGFloat cgFloatComponents[4] { r, g, b, a };
+
+    switch (colorSpace) {
+    case ColorSpace::SRGB:
+        return CGColorCreate(sRGBColorSpaceRef(), cgFloatComponents);
+    case ColorSpace::DisplayP3:
+        return CGColorCreate(displayP3ColorSpaceRef(), cgFloatComponents);
+    case ColorSpace::LinearRGB:
+        // FIXME: Do we ever create CGColorRefs in these spaces? It may only be ImageBuffers.
+        return CGColorCreate(sRGBColorSpaceRef(), cgFloatComponents);
     }
 
-    color.getRGBA(components[0], components[1], components[2], components[3]);
-    return CGColorCreate(sRGBColorSpaceRef(), components);
+    ASSERT_NOT_REACHED();
+    return nullptr;
 }
 
 CGColorRef cachedCGColor(const Color& color)
index deeb91f..87310d5 100644 (file)
@@ -70,11 +70,8 @@ CGGradientRef Gradient::platformGradient()
         if (stop.color.isExtended())
             hasExtendedColors = true;
 
-        float r;
-        float g;
-        float b;
-        float a;
-        stop.color.getRGBA(r, g, b, a);
+        auto [colorSpace, components] = stop.color.colorSpaceAndComponents();
+        auto [r, g, b, a] = components;
         colorComponents.uncheckedAppend(r);
         colorComponents.uncheckedAppend(g);
         colorComponents.uncheckedAppend(b);
index 6db87fd..7eef270 100644 (file)
@@ -404,7 +404,7 @@ bool FELighting::drawLighting(Uint8ClampedArray& pixels, int width, int height)
     data.widthDecreasedByOne = width - 1;
     data.heightDecreasedByOne = height - 1;
     
-    FloatComponents lightColor = (operatingColorSpace() == ColorSpace::LinearRGB) ? sRGBColorToLinearComponents(m_lightingColor) : FloatComponents(m_lightingColor);
+    FloatComponents lightColor = (operatingColorSpace() == ColorSpace::LinearRGB) ? rgbToLinearComponents(m_lightingColor.toSRGBAComponentsLossy()) : m_lightingColor.toSRGBAComponentsLossy();
     paintingData.initialLightingData.colorVector = FloatPoint3D(lightColor.components[0], lightColor.components[1], lightColor.components[2]);
     m_lightSource->initPaintingData(*this, paintingData);
 
index 7f8974c..f4f9cb9 100644 (file)
@@ -111,8 +111,7 @@ bool FilterOperations::transformColor(Color& color) const
     if (color.isSemantic())
         return false;
 
-    FloatComponents components;
-    color.getRGBA(components.components[0], components.components[1], components.components[2], components.components[3]);
+    FloatComponents components = color.toSRGBAComponentsLossy();
 
     for (auto& operation : m_operations) {
         if (!operation->transformColor(components))
@@ -131,8 +130,7 @@ bool FilterOperations::inverseTransformColor(Color& color) const
     if (color.isSemantic())
         return false;
 
-    FloatComponents components;
-    color.getRGBA(components.components[0], components.components[1], components.components[2], components.components[3]);
+    FloatComponents components = color.toSRGBAComponentsLossy();
 
     for (auto& operation : m_operations) {
         if (!operation->inverseTransformColor(components))
index 43db5c3..268e722 100644 (file)
@@ -36,18 +36,8 @@ Color::Color(const GdkRGBA& c)
 
 Color::operator GdkRGBA() const
 {
-    if (isExtended()) {
-        auto asRGBA = toSRGBAComponentsLossy();
-        return { asRGBA.components[0], asRGBA.components[1], asRGBA.components[2], asRGBA.components[3] };
-    }
-
-#if USE(GTK4)
-    float red, green, blue, alpha;
-#else
-    double red, green, blue, alpha;
-#endif
-    getRGBA(red, green, blue, alpha);
-    return { red, green, blue, alpha };
+    auto [r, g, b, a] = toSRGBAComponentsLossy();
+    return { r, g, b, a };
 }
 
 }
index fba1120..e6699bf 100644 (file)
@@ -253,8 +253,8 @@ void TextureMapperGL::drawBorder(const Color& color, float width, const FloatRec
     Ref<TextureMapperShaderProgram> program = data().getShaderProgram(TextureMapperShaderProgram::SolidColor);
     glUseProgram(program->programID());
 
-    float r, g, b, a;
-    Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a);
+    // FIXME: Do the premultiply on FloatComponents directly.
+    auto [r, g, b, a] = Color(premultipliedARGBFromColor(color)).toSRGBAComponentsLossy();
     glUniform4f(program->colorLocation(), r, g, b, a);
     glLineWidth(width);
 
@@ -276,13 +276,8 @@ void TextureMapperGL::drawNumber(int number, const Color& color, const FloatPoin
     cairo_t* cr = cairo_create(surface);
 
     // Since we won't swap R+B when uploading a texture, paint with the swapped R+B color.
-    if (color.isExtended())
-        cairo_set_source_rgba(cr, color.asExtended().blue(), color.asExtended().green(), color.asExtended().red(), color.asExtended().alpha());
-    else {
-        float r, g, b, a;
-        color.getRGBA(r, g, b, a);
-        cairo_set_source_rgba(cr, b, g, r, a);
-    }
+    auto [r, g, b, a] = color.toSRGBAComponentsLossy();
+    cairo_set_source_rgba(cr, b, g, r, a);
 
     cairo_rectangle(cr, 0, 0, width, height);
     cairo_fill(cr);
@@ -420,8 +415,8 @@ static void prepareFilterProgram(TextureMapperShaderProgram& program, const Filt
             break;
         case 1:
             // Second pass: we need the shadow color and the content texture for compositing.
-            float r, g, b, a;
-            Color(premultipliedARGBFromColor(shadow.color())).getRGBA(r, g, b, a);
+            // FIXME: Do the premultiply on FloatComponents directly.
+            auto [r, g, b, a] = Color(premultipliedARGBFromColor(shadow.color())).toSRGBAComponentsLossy();
             glUniform4f(program.colorLocation(), r, g, b, a);
             glUniform2f(program.blurRadiusLocation(), 0, shadow.stdDeviation() / float(size.height()));
             glUniform2f(program.shadowOffsetLocation(), 0, 0);
@@ -683,8 +678,8 @@ void TextureMapperGL::drawSolidColor(const FloatRect& rect, const Transformation
     Ref<TextureMapperShaderProgram> program = data().getShaderProgram(options);
     glUseProgram(program->programID());
 
-    float r, g, b, a;
-    Color(premultipliedARGBFromColor(color)).getRGBA(r, g, b, a);
+    // FIXME: Do the premultiply on FloatComponents directly.
+    auto [r, g, b, a] = Color(premultipliedARGBFromColor(color)).toSRGBAComponentsLossy();
     glUniform4f(program->colorLocation(), r, g, b, a);
     if (a < 1 && isBlendingAllowed)
         flags |= ShouldBlend;
index e9fe49b..0bcb592 100644 (file)
@@ -72,11 +72,7 @@ void Gradient::generateGradient(ID2D1RenderTarget* renderTarget)
     Vector<D2D1_GRADIENT_STOP> gradientStops;
     // FIXME: Add support for ExtendedColor.
     for (auto stop : m_stops) {
-        float r;
-        float g;
-        float b;
-        float a;
-        stop.color.getRGBA(r, g, b, a);
+        auto [r, g, b, a] stop.color.toSRGBAComponentsLossy();
         gradientStops.append(D2D1::GradientStop(stop.offset, D2D1::ColorF(r, g, b, a)));
     }
 
index 1c1d966..86bdf74 100644 (file)
@@ -170,24 +170,6 @@ void GraphicsContext::drawFocusRing(const Vector<FloatRect>& rects, float width,
     CGContextRestoreGState(context);
 }
 
-// Pulled from GraphicsContextCG
-static void setCGStrokeColor(CGContextRef context, const Color& color)
-{
-    CGFloat red, green, blue, alpha;
-    color.getRGBA(red, green, blue, alpha);
-    CGContextSetRGBStrokeColor(context, red, green, blue, alpha);
-}
-
-static const Color& spellingPatternColor() {
-    static const Color spellingColor(255, 0, 0);
-    return spellingColor;
-}
-
-static const Color& grammarPatternColor() {
-    static const Color grammarColor(0, 128, 0);
-    return grammarColor;
-}
-
 void GraphicsContext::drawDotsForDocumentMarker(const FloatRect& rect, DocumentMarkerLineStyle style)
 {
     if (paintingDisabled())
@@ -216,8 +198,11 @@ void GraphicsContext::drawDotsForDocumentMarker(const FloatRect& rect, DocumentM
     CGContextRef context = platformContext();
     CGContextSaveGState(context);
 
-    const Color& patternColor = style.mode == DocumentMarkerLineStyle::Mode::Grammar ? grammarPatternColor() : spellingPatternColor();
-    setCGStrokeColor(context, patternColor);
+    static constexpr SimpleColor spellingPatternColor { makeRGB(255, 0, 0) };
+    static constexpr SimpleColor grammarPatternColor { makeRGB(0, 128, 0) };
+
+    const SimpleColor& patternColor = style.mode == DocumentMarkerLineStyle::Mode::Grammar ? grammarPatternColor : spellingPatternColor;
+    CGContextSetRGBStrokeColor(context, patternColor.redComponent(), patternColor.greenComponent(), patternColor.blueComponent(), patternColor.alphaComponent());
 
     CGAffineTransform userToBase = getUserToBaseCTM(context);
     CGPoint phase = CGPointApplyAffineTransform(point, userToBase);
index c5c6ad5..4c8f6ff 100644 (file)
@@ -63,7 +63,7 @@ bool textColorIsLegibleAgainstBackgroundColor(const Color& textColor, const Colo
 {
     // Uses the WCAG 2.0 definition of legibility: a contrast ratio of 4.5:1 or greater.
     // https://www.w3.org/TR/WCAG20/#visual-audio-contrast-contrast
-    return contrastRatio(textColor, backgroundColor) > 4.5;
+    return contrastRatio(textColor.toSRGBAComponentsLossy(), backgroundColor.toSRGBAComponentsLossy()) > 4.5;
 }
 
 static Color adjustColorForVisibilityOnBackground(const Color& textColor, const Color& backgroundColor)
index f5f0aef..94f2dba 100644 (file)
@@ -1,3 +1,18 @@
+2020-05-19  Sam Weinig  <weinig@apple.com>
+
+        Remove almost always incorrect Color::getRGBA
+        https://bugs.webkit.org/show_bug.cgi?id=212059
+
+        Reviewed by Darin Adler and Simon Fraser.
+
+        * UIProcess/API/wpe/WebKitColor.cpp:
+        (webkitColorFillFromWebCoreColor):
+        * UIProcess/gtk/ViewGestureControllerGtk.cpp:
+        (WebKit::ViewGestureController::beginSwipeGesture):
+        * WebProcess/WebPage/WebFrame.cpp:
+        (WebKit::WebFrame::getDocumentBackgroundColor):
+        Replace Color::getRGBA with Color::toSRGBAComponentsLossy()
+
 2020-05-19  Simon Fraser  <simon.fraser@apple.com>
 
         Use an ObjectIdentifier<> for DisplayLink observer IDs
index b818f95..d222783 100644 (file)
@@ -82,8 +82,7 @@ void webkitColorFillFromWebCoreColor(const WebCore::Color& webCoreColor, WebKitC
 {
     RELEASE_ASSERT(webCoreColor.isValid());
 
-    double r, g, b, a;
-    webCoreColor.getRGBA(r, g, b, a);
+    auto [r, g, b, a] = webCoreColor.toSRGBAComponentsLossy();
     color->red = r;
     color->green = g;
     color->blue = b;
index 35359bd..a163538 100644 (file)
@@ -337,8 +337,7 @@ void ViewGestureController::beginSwipeGesture(WebBackForwardListItem* targetItem
         if (color.isValid()) {
             m_backgroundColorForCurrentSnapshot = color;
             if (!m_currentSwipeSnapshotPattern) {
-                double red, green, blue, alpha;
-                color.getRGBA(red, green, blue, alpha);
+                auto [red, green, blue, alpha] = color.toSRGBAComponentsLossy();
                 m_currentSwipeSnapshotPattern = adoptRef(cairo_pattern_create_rgba(red, green, blue, alpha));
             }
         }
index d78b124..7a05913 100644 (file)
@@ -660,7 +660,11 @@ bool WebFrame::getDocumentBackgroundColor(double* red, double* green, double* bl
     if (!bgColor.isValid())
         return false;
 
-    bgColor.getRGBA(*red, *green, *blue, *alpha);
+    auto [r, g, b, a] = bgColor.toSRGBAComponentsLossy();
+    *red = r;
+    *green = g;
+    *blue = b;
+    *alpha = a;
     return true;
 }