[webkit-reviews] review granted: [Bug 233153] Implement parsing and animation support for ray() shape accepted by offset-path : [Attachment 444696] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Kiet Ho <tho22 at apple.com>'s request
for review:
Bug 233153: Implement parsing and animation support for ray() shape accepted by
offset-path
https://bugs.webkit.org/show_bug.cgi?id=233153

Attachment 444696: Patch

https://bugs.webkit.org/attachment.cgi?id=444696&action=review




--- Comment #5 from Dean Jackson <dino at apple.com> ---
Comment on attachment 444696
  --> https://bugs.webkit.org/attachment.cgi?id=444696
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-comp
uted.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?