Bug 180620

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.

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

Created attachment 331165 [details] Patch

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.

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.

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.

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.

(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

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.