[webkit-reviews] review granted: [Bug 217320] Add non-animated support for the CSS rotate property : [Attachment 410527] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Antoine Quint <graouts at apple.com>'s
request for review:
Bug 217320: Add non-animated support for the CSS rotate property
https://bugs.webkit.org/show_bug.cgi?id=217320

Attachment 410527: Patch

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




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

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

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:658
> +    if (!rendererCanBeTransformed(renderer) || !rotate)

swap these?

> Source/WebCore/css/CSSValueKeywords.in:1143
> +// x
> +// y

Typically you'd leave these ones because they were already present, and they
come first in the file (and comment the rotate versions)

> Source/WebCore/css/TransformFunctions.cpp:476
> +    return RotateTransformOperation::create(x, y, z, angle, type);

Are the x, y, z values normalized after parsing?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2141
> +    RefPtr<CSSValue> parsedValue;

This can go inside the loop.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2168
> +	   if (parsedValue) {
> +	       // If we've encountered an axis identifier, then this valus is
invalid.
> +	       if (axisIdentifier)
> +		   return nullptr;
> +	       list->append(*parsedValue);
> +	   } else {
> +	       // Then, attempt to parse an angle. We try this as a fallback
rather than the first option because
> +	       // a unitless 0 angle would be consumed as an angle.
> +	       parsedValue = consumeAngle(range, cssParserMode,
UnitlessQuirk::Forbid);
> +	       if (parsedValue) {
> +		   // If we had already parsed an angle or numbers but not 3 in
a row, this value is invalid.
> +		   if (angle || (list->length() && list->length() != 3))
> +		       return nullptr;
> +		   angle = parsedValue;
> +	       } else {
> +		   // Finally, attempt to parse one of the axis identifiers.
> +		   parsedValue = consumeIdent<CSSValueX, CSSValueY,
CSSValueZ>(range);
> +		   // If we failed to find one of those identifiers or one was
already specified, or we'd previously
> +		   // encountered numbers to specify a rotation axis, then this
value is invalid.
> +		   if (!parsedValue || axisIdentifier || list->length())
> +		       return nullptr;
> +		   axisIdentifier = parsedValue;
> +	       }
> +	   }

These might read easier with "continue" statements rather than "else".

e.g.

// 1. Attempt to parse a number.
parsedValue = consumeNumber();
if (parsedValue) {
  // If we've encountered an axis...
  if (axisIdentifier)
    return nullptr;

  list->append(*parsedValue);
  continue;
}

// 2. Attempt to parse an angle....