Bug 180620 – Improve ExtendedColor support

• Status: ASSIGNED
• Component: Platform

[Darin Adler]

Improve ExtendedColor support

[Darin Adler]

Created attachment 328905 [details]
Patch

[Darin Adler]

Created attachment 328951 [details]
Patch

[Darin Adler]

Created attachment 328952 [details]
Patch

[Darin Adler]

Created attachment 329082 [details]
Patch

[Darin Adler]

Created attachment 329087 [details]
Patch

[EWS Watchlist]

Comment on attachment 329087 [details]
Patch

Attachment 329087 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5626713

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329091 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329087 [details]
Patch

Attachment 329087 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5626727

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329092 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329087 [details]
Patch

Attachment 329087 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5626709

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 329093 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 329606 [details]
Patch

[Darin Adler]

Created attachment 329607 [details]
Patch

[Darin Adler]

Created attachment 329608 [details]
Patch

[Darin Adler]

Created attachment 329612 [details]
Patch

[Darin Adler]

Created attachment 329614 [details]
Patch

[Darin Adler]

Created attachment 329618 [details]
Patch

[Darin Adler]

Created attachment 329621 [details]
Patch

[EWS Watchlist]

Comment on attachment 329621 [details]
Patch

Attachment 329621 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5705378

New failing tests:
fast/forms/file/entries-api/webkitdirectory-open-panel.html

[EWS Watchlist]

Created attachment 329623 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329621 [details]
Patch

Attachment 329621 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5705521

New failing tests:
svg/custom/dominant-baseline-hanging.svg
svg/W3C-SVG-1.1/masking-mask-01-b.svg
compositing/webgl/webgl-nonpremultiplied-blend.html
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
fast/forms/file/entries-api/webkitdirectory-open-panel.html
svg/custom/text-rotated-gradient.svg

[EWS Watchlist]

Created attachment 329624 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 329631 [details]
Patch

[EWS Watchlist]

Comment on attachment 329631 [details]
Patch

Attachment 329631 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5711673

New failing tests:
compositing/webgl/webgl-nonpremultiplied-blend.html

[EWS Watchlist]

Created attachment 329634 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Darin Adler]

Created attachment 329635 [details]
Patch

[EWS Watchlist]

Comment on attachment 329635 [details]
Patch

Attachment 329635 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5713114

New failing tests:
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
svg/custom/dominant-baseline-hanging.svg
svg/custom/gradient-stroke-width.svg
svg/custom/js-update-gradient.svg
fast/forms/input-disabled-color.html

[EWS Watchlist]

Created attachment 329638 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329635 [details]
Patch

Attachment 329635 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5713159

New failing tests:
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
svg/custom/dominant-baseline-hanging.svg
svg/custom/gradient-stroke-width.svg
svg/custom/js-update-gradient.svg
fast/forms/input-disabled-color.html

[EWS Watchlist]

Created attachment 329639 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[EWS Watchlist]

Comment on attachment 329635 [details]
Patch

Attachment 329635 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5713203

New failing tests:
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
svg/custom/dominant-baseline-hanging.svg
svg/custom/gradient-stroke-width.svg
svg/custom/js-update-gradient.svg
fast/forms/input-disabled-color.html

[EWS Watchlist]

Created attachment 329640 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 329635 [details]
Patch

Attachment 329635 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5713168

New failing tests:
svg/W3C-SVG-1.1/pservers-grad-05-b.svg
svg/custom/dominant-baseline-hanging.svg
svg/custom/gradient-stroke-width.svg
svg/custom/js-update-gradient.svg
fast/forms/input-disabled-color.html

[EWS Watchlist]

Created attachment 329641 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Darin Adler]

Created attachment 331031 [details]
Patch

[Darin Adler]

Created attachment 331036 [details]
Patch

[Darin Adler]

Created attachment 331058 [details]
Patch

[Darin Adler]

I think the patch is ready for review now; but I could land it in much smaller pieces if reviewers think I should. Would probably make the process take weeks, but that’s OK with me.

[Darin Adler]

Created attachment 331159 [details]
Patch

[Darin Adler]

(Accidentally uploaded a patch without the new files last time.)

[Darin Adler]

Created attachment 331165 [details]
Patch

[Darin Adler]

Dean, Simon, anyone, would love to get your feedback on this patch eventually; finally got it all working three weeks ago. Some combination of:

1) Review and encouragement to finish and land.

2) Comments about what seems wrong or better ideas you may have, so I can continue to refine this or decide to abandon it.

3) Specific suggestion about the need to break it into smaller pieces.

I am open to any of that.

[Simon Fraser (smfr)]

Comment on attachment 331165 [details]
Patch

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

I ran out of time half-way though, but I really thing this needs to be broken up in to probably many pieces.

> Source/WebCore/css/CSSValuePool.cpp:134
> +    return *m_fontFamilyValueCache.ensure({ familyName, isFromSystemID }, [&familyName, isFromSystemID] {
> +        return CSSPrimitiveValue::create(CSSFontFamily { familyName, isFromSystemID });
> +    }).iterator->value;

Not color-related. Do in separate patch.

> Source/WebCore/css/CSSValuePool.cpp:151
> +    return m_fontFaceValueCache.ensure(string, [&string] () -> RefPtr<CSSValueList> {
> +        auto result = CSSParser::parseSingleValue(CSSPropertyFontFamily, string);
> +        if (!is<CSSValueList>(result))
> +            return nullptr;
> +        // FIXME: Make downcast work on RefPtr, remove the get() below, and save one reference count churn.
> +        return downcast<CSSValueList>(result.get());
> +    }).iterator->value;

Not color-related. Do in separate patch.

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:462
> +static std::optional<SimpleColor> finishParsingHexColor(uint32_t value, unsigned length)
> +{
> +    switch (length) {
> +    case 3:
> +        // #abc converts to #aabbcc
> +        return SimpleColor { 0xFF000000
> +            | (value & 0xF00) << 12 | (value & 0xF00) << 8
> +            | (value & 0xF0) << 8 | (value & 0xF0) << 4
> +            | (value & 0xF) << 4 | (value & 0xF) };
> +    case 4:
> +        // #abcd converts to ddaabbcc since alpha bytes are the high bytes.
> +        return SimpleColor { (value & 0xF) << 28 | (value & 0xF) << 24
> +            | (value & 0xF000) << 8 | (value & 0xF000) << 4
> +            | (value & 0xF00) << 4 | (value & 0xF00)
> +            | (value & 0xF0) | (value & 0xF0) >> 4 };
> +    case 6:
> +        return SimpleColor { 0xFF000000 | value };
> +    case 8:
> +        // We parsed the values into RGBA order, but we store them in ARGB order.
> +        return SimpleColor { value << 24 | value >> 8 };
> +    }
> +    return std::nullopt;
> +}
> +
> +template<typename CharacterType> static std::optional<SimpleColor> fastParseHexColor(const CharacterType* characters, unsigned length)

Would be nice to land parsing changes separately from the other color stuff is possible, to make it easier to determine whether regressions are parsing regressions or other.

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:577
> +    if (auto color = fastParseNumericColor(characters, length, inQuirksMode))
> +        return color;
> +    return fastParseNamedColor(characters, length);

I'm glad we're not trying named colors first now. This makes much more sense.

> Source/WebCore/css/parser/CSSParserFastPaths.cpp:1403
> -    RefPtr<CSSValue> result = parseSimpleLengthValue(propertyID, string, parserMode);
> -    if (result)
> +    if (auto result = parseSimpleLengthValue(propertyID, string, parserMode))
>          return result;
>      if (propertyID == CSSPropertyCaretColor)
>          return parseCaretColor(string, parserMode);
>      if (isColorPropertyID(propertyID))
>          return parseColor(string, parserMode);
> -    result = parseKeywordValue(propertyID, string, parserMode);
> -    if (result)
> -        return result;
> -    result = parseSimpleTransform(propertyID, string);
> -    if (result)
> +    if (auto result = parseKeywordValue(propertyID, string, parserMode))
>          return result;
> -    return nullptr;
> +    return parseSimpleTransform(propertyID, string);

Not color-related. Do in separate patch.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:-65
> -#include "FloatQuad.h"
> -#include "HTMLImageElement.h"
> -#include "HTMLVideoElement.h"
> -#include "ImageBitmap.h"
>  #include "ImageBuffer.h"
>  #include "ImageData.h"
>  #include "InspectorInstrumentation.h"
>  #include "Path2D.h"
> -#include "RenderElement.h"
> -#include "RenderImage.h"
> -#include "RenderLayer.h"
>  #include "RenderTheme.h"
> -#include "SecurityOrigin.h"
> -#include "StrokeStyleApplier.h"
>  #include "StyleProperties.h"
>  #include "StyleResolver.h"
>  #include "TextMetrics.h"
>  #include "TextRun.h"
>  #include <wtf/CheckedArithmetic.h>
>  #include <wtf/MathExtras.h>
> -#include <wtf/NeverDestroyed.h>
>  #include <wtf/text/StringBuilder.h>
> -#include <wtf/text/TextStream.h>
> -
> -#if USE(CG) && !PLATFORM(IOS)
> -#include <ApplicationServices/ApplicationServices.h>
> -#endif

!

> Source/WebCore/inspector/InspectorCanvas.cpp:163
> -    m_currentActions->addItem(action);
> +    m_currentActions->addItem(action.ptr());
>  
>  #if ENABLE(WEBGL)
>      if (is<WebGLRenderingContext>(m_context) && shouldSnapshotWebGLAction(name))
> -        m_actionNeedingSnapshot = action;
> +        m_actionNeedingSnapshot = WTFMove(action);
>  #endif

More stuff that should land separately.

> Source/WebCore/inspector/InspectorCanvas.cpp:417
> -static RefPtr<JSON::ArrayOf<double>> buildArrayForAffineTransform(const AffineTransform& affineTransform)
> +static Ref<JSON::ArrayOf<double>> buildArrayForAffineTransform(const AffineTransform& affineTransform)
>  {
> -    RefPtr<JSON::ArrayOf<double>> array = JSON::ArrayOf<double>::create();
> +    auto array = JSON::ArrayOf<double>::create();

This whole set of changes is just auto stuff and cleanup. Should be separate.

> Source/WebCore/inspector/InspectorCanvas.h:104
> -    RefPtr<Inspector::Protocol::Recording::InitialState> buildInitialState();
> -    RefPtr<JSON::ArrayOf<JSON::Value>> buildAction(const String&, Vector<RecordCanvasActionVariant>&& = { });
> -    RefPtr<JSON::ArrayOf<JSON::Value>> buildArrayForCanvasGradient(const CanvasGradient&);
> -    RefPtr<JSON::ArrayOf<JSON::Value>> buildArrayForCanvasPattern(const CanvasPattern&);
> -    RefPtr<JSON::ArrayOf<JSON::Value>> buildArrayForImageData(const ImageData&);
> +    Ref<Inspector::Protocol::Recording::InitialState> buildInitialState();
> +    Ref<JSON::ArrayOf<JSON::Value>> buildAction(const String&, Vector<RecordCanvasActionVariant>&& = { });
> +    Ref<JSON::ArrayOf<JSON::Value>> buildArrayForCanvasGradient(const CanvasGradient&);
> +    Ref<JSON::ArrayOf<JSON::Value>> buildArrayForCanvasPattern(const CanvasPattern&);
> +    Ref<JSON::ArrayOf<JSON::Value>> buildArrayForImageData(const ImageData&);

Land separately.

[Darin Adler]

OK, started by excerpting all the things Simon called out above (except the CSS parser part, bigger job to do that) and other independent tweaks into a new patch/bug.

[Darin Adler]

This patch has lots of different ideas about improving Color. Some of them are things done already recently by Sam and others. Some of them are things where we did something better. But other things here are still good ideas.

[Sam Weinig]

(In reply to Darin Adler from comment #45)
> This patch has lots of different ideas about improving Color. Some of them
> are things done already recently by Sam and others. Some of them are things
> where we did something better. But other things here are still good ideas.

Oh man, so much overlap! I did a pass of it and here are some things I think we should still do from it (I am probably missing some, but I wanted to write them down so I wouldn't forget):

- Rename makeSimpleColor to makeSimpleColorFromRGBABytes (for 0-255 based input)
- Rename makeSimpleColorFromFloats to makeSimpleColorFromRGBA (for 0.0f-1.0f based input)
- Add non-clamping version of makeSimpleColorFromRGBABytes that takes uint8_ts
- Move roundAndClampColorChannel to SVGAnimatedColor.cpp
- Remove uses of code that requires NSDeviceRGBColorSpace, replacing with colorFromNSColor() in most cases
- Lots of the API tests can be added as is (or slightly modified for different names
- Use same overall structure for nsColor()/CSSValuePool::createColorValue() as leakCGColor() (e.g. switch + TinyLRUCache)
- Split makePremultipliedSimpleColor into makePremultipliedFlooringSimpleColor/makePremultipliedCeilingSimpleColor


Additional things I think we should do that weren't in the patch directly:
- Time to replace uses of nextafter(256, 0) base conversion to int (chop and truncates) with 255 based conversion (clamp and round).
- Move color modifying functions (e.g. blend, invert, lighten, etc.) outside of the Color class itself and into ColorUtilities.
- Requiring passing the "working space" colorspace to blending / gradient / animation code 
- Remove makeSimpleColorFromHSLA and makeSimpleColorFromCMYKA, move / use conversion code in ColorUtilities
- Consolidate/rationalize all the vaiious methods of determing relative differences (e.g. isDark(), lightness(), differenceSquared()) 
- Need to add encoding/decoding of semantic bit to Color coder
- Should delegate to ExtendedColor/SimpleColor for coding
- Rename colorWithAlpha to colorWithOverriddenAlpha

[Darin Adler]

Seems like I should close this soon. I think almost everything has been done same or better by Sam or some earlier smaller patches by me.