[webkit-reviews] review granted: [Bug 232752] Implement parsing and animation support for offset-rotate : [Attachment 443473] Patch. Fix build for platforms using CMake

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Kiet Ho <tho22 at apple.com>'s
request for review:
Bug 232752: Implement parsing and animation support for offset-rotate
https://bugs.webkit.org/show_bug.cgi?id=232752

Attachment 443473: Patch. Fix build for platforms using CMake

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




--- Comment #7 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 443473
  --> https://bugs.webkit.org/attachment.cgi?id=443473
Patch. Fix build for platforms using CMake

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

Looks like a good start to the feature.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2510
> +	   if (rotation.angle() != 0.0)

Is “auto 0” supposed to serialize to just “auto”?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4046
> +    auto list = CSSValueList::createSpaceSeparated();

Did you consider making a new subclass of CSSValue for this? It always feels
icky to me pulling out the 1th item from a list and assuming it has some
particular meaning. Or, isn’t there already a CSSValue that holds pairs?

> Source/WebCore/rendering/style/OffsetRotation.h:43
> +    double m_angle;

I don’t quite understand this. How do you differentiate the case between
“offset-rotation: auto” and “offset-rotation: auto 30deg”? It seems like you’d
need optional<double> m_angle.

Also why double and not float?

> Source/WebCore/style/StyleBuilderConverter.h:1616
> +    for (auto element : list) {

��

> Source/WebCore/style/StyleBuilderConverter.h:1639
> +	   angle += 180.0;

+ 180, rather than * -1?