Bug 233153

Created attachment 444511 [details] WIP

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

Created attachment 444696 [details] Patch

<rdar://problem/85611929>

Comment on attachment 444696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444696&action=review > Source/WebCore/css/parser/CSSPropertyParser.cpp:2807 > + RefPtr<CSSPrimitiveValue> isContaining; > + while (!args.atEnd()) { > + if (!angle && (angle = consumeAngle(args, context.mode, UnitlessQuirk::Forbid, UnitlessZeroQuirk::Forbid))) > + continue; > + > + if (!size && (size = consumeIdent<CSSValueClosestSide, CSSValueClosestCorner, CSSValueFarthestSide, CSSValueFarthestCorner, CSSValueSides>(args))) > + continue; > + > + if (!isContaining && (isContaining = consumeIdent<CSSValueContain>(args))) > + continue; > + > + return nullptr; > + } > + > + // <angle> and <size> must be present. > + if (!angle || !size) > + return nullptr; > + > + return CSSRayValue::create(angle.releaseNonNull(), size.releaseNonNull(), isContaining); Given that you end up passing a bool for the isContaining parameter, I wonder if you ever need the RefPtr<CSSPrimitiveValue> for it. Wouldn't it work to have a bool variable, and simply test that consumeIdent<CSSValueContain> returns a value, then set the bool to true before calling continue? > Source/WebCore/style/StyleBuilderConverter.h:663 > + RayPathOperation::Size size = RayPathOperation::Size::ClosestCorner; > + switch (rayValue.size()->valueID()) { > + case CSSValueClosestCorner: > + size = RayPathOperation::Size::ClosestCorner; > + break; > + case CSSValueClosestSide: > + size = RayPathOperation::Size::ClosestSide; > + break; > + case CSSValueFarthestCorner: > + size = RayPathOperation::Size::FarthestCorner; > + break; > + case CSSValueFarthestSide: > + size = RayPathOperation::Size::FarthestSide; > + break; > + case CSSValueSides: > + size = RayPathOperation::Size::Sides; > + break; > + default: > + ASSERT_NOT_REACHED(); > + return nullptr; > + } Not important, but I see people tend to use immediately called lambdas for initialisation of variables? e.g. auto size = [] (CSSValueID valueID) -> RayPathOperation::Size { switch (valueID) { case CSSValueClosestCorner: return ClosestCorner; ... }(rayValue.size()->valueID()); Don't make the change though. This is fine. > LayoutTests/imported/w3c/web-platform-tests/css/motion/parsing/offset-path-computed.html:22 > -test_computed_value("offset-path", "ray(calc(180deg - 45deg) farthest-side)", "ray(calc(135deg) farthest-side)"); > +test_computed_value("offset-path", "ray(calc(180deg - 45deg) farthest-side)", "ray(135deg farthest-side)"); This is making a change to an imported test? Was it intentional? What happens when we update WPT?

Comment on attachment 444696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444696&action=review >> Source/WebCore/css/parser/CSSPropertyParser.cpp:2807 >> + return CSSRayValue::create(angle.releaseNonNull(), size.releaseNonNull(), isContaining); > > Given that you end up passing a bool for the isContaining parameter, I wonder if you ever need the RefPtr<CSSPrimitiveValue> for it. Wouldn't it work to have a bool variable, and simply test that consumeIdent<CSSValueContain> returns a value, then set the bool to true before calling continue? You're right, I'll change it to just a simple bool. >> LayoutTests/imported/w3c/web-platform-tests/css/motion/parsing/offset-path-computed.html:22 >> +test_computed_value("offset-path", "ray(calc(180deg - 45deg) farthest-side)", "ray(135deg farthest-side)"); > > This is making a change to an imported test? Was it intentional? What happens when we update WPT? This change is exported here: https://github.com/web-platform-tests/wpt/pull/31658. Once this patch is merged, the bot will automatically merge the WPT pull request.

Created attachment 444859 [details] Patch

Committed r286086 (244473@main): <https://commits.webkit.org/244473@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 444859 [details].