[webkit-reviews] review denied: [Bug 216997] Add non-animated support for the CSS translate property : [Attachment 409749] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Antoine Quint
<graouts at webkit.org>'s request for review:
Bug 216997: Add non-animated support for the CSS translate property
https://bugs.webkit.org/show_bug.cgi?id=216997

Attachment 409749: Patch

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




--- Comment #6 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 409749
  --> https://bugs.webkit.org/attachment.cgi?id=409749
Patch

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

You need to trigger stacking context too (and test that) - see
hasTransformRelatedProperty().

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:612
> +    return CSSValuePool::singleton().createIdentifierValue(CSSValueNone);

I think this patch needs to implement enough to pass some WPT tests and
therefore implement this.

> Source/WebCore/css/TransformFunctions.cpp:356
> +    // FIXME: what is this used for exactly?
> +    UNUSED_PARAM(value);
> +    UNUSED_PARAM(conversionData);
> +    return nullptr;

Not great to see this in a patch.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2043
> +	   return list;

Shouldn't this return nullptr since it's invalid?

> Source/WebCore/rendering/style/RenderStyle.cpp:1403
> +    if (TransformOperation* translate =
m_rareNonInheritedData->translate.get())
> +	   translate->apply(transform, boundingBox.size());
> +

Is this order right w.r.t. the transform property? I thought the spec described
this and it needs to be tested.