[webkit-reviews] review granted: [Bug 233109] Implement parsing and animation support for offset shorthand : [Attachment 451727] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 233109: Implement parsing and animation support for offset shorthand
https://bugs.webkit.org/show_bug.cgi?id=233109

Attachment 451727: Patch

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




--- Comment #10 from Dean Jackson <dino at apple.com> ---
Comment on attachment 451727
  --> https://bugs.webkit.org/attachment.cgi?id=451727
Patch

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2640
> +    {
> +	   auto innerList = CSSValueList::createSpaceSeparated();
> +	   innerList->append(valueForPositionOrAuto(style,
style.offsetPosition()));
> +	   innerList->append(valueForPathOperation(style, style.offsetPath(),
SVGPathConversion::ForceAbsolute));
> +	  
innerList->append(CSSValuePool::singleton().createValue(style.offsetDistance(),
style));
> +	   innerList->append(valueForOffsetRotate(style.offsetRotate()));
> +
> +	   outerList->append(WTFMove(innerList));
> +    }

Any reason for this being in its own  scope?

> Source/WebCore/css/StyleProperties.cpp:466
> +static bool isAutoKeyword(const CSSValue& value)
> +{
> +    if (!is<CSSPrimitiveValue>(value))
> +	   return false;
> +
> +    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
> +    return primitiveValue.isValueID() && primitiveValue.valueID() ==
CSSValueAuto;
> +}

Is this really the first/only time we're checking for this keyword in the file?

> Source/WebCore/css/StyleProperties.cpp:508
> +	       if (!offsetDistance.value() ||
!is<CSSPrimitiveValue>(offsetDistance.value()))

Maybe a local variable for offsetDistance.value() to avoid calling it multiple
times?

> Source/WebCore/css/StyleProperties.cpp:527
> +	       if (!offsetRotate.value() ||
!is<CSSOffsetRotateValue>(offsetRotate.value()))

Same with this .value()