[webkit-reviews] review denied: [Bug 122524] [CSS Shapes] Shape-Margin should be animatable : [Attachment 213720] Initial patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Bear Travis <betravis at adobe.com>'s
request for review:
Bug 122524: [CSS Shapes] Shape-Margin should be animatable
https://bugs.webkit.org/show_bug.cgi?id=122524

Attachment 213720: Initial patch
https://bugs.webkit.org/attachment.cgi?id=213720&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213720&action=review


> Source/WebCore/ChangeLog:3155
> +	   [CSS Shapes] Basic shapes should be animatable for shape-outside
> +	   https://bugs.webkit.org/show_bug.cgi?id=122343
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: fast/shapes/shape-outside-floats/shape-outside-animation.html
> +
> +	   Add shape outside to the list of animatable properties. The
infrastructure
> +	   is already in place for animating basic shapes on shape-inside and
clipping
> +	   paths. See https://bugs.webkit.org/show_bug.cgi?id=101854.
> +
> +	   * page/animation/CSSPropertyAnimation.cpp:
> +	  
(WebCore::CSSPropertyAnimationWrapperMap::CSSPropertyAnimationWrapperMap):

Extra change log here. Oops!

> Source/WebCore/rendering/RenderBox.cpp:391
>      // FIXME: A future optimization would do a deep comparison for equality.
(bug 100811)

This comment makes more sense just before the "==". It’s a little strange to
have it paragraphed with code that just sets up shapeOutside locals.

> Source/WebCore/rendering/RenderBox.cpp:393
> +    const ShapeValue* oldShapeOutside = oldStyle ? oldStyle->shapeOutside()
: 0;

Please use nullptr instead of 0.

> Source/WebCore/rendering/RenderBox.cpp:396
> +    const Length shapeMargin = style->shapeMargin();
> +    const Length oldShapeMargin = oldStyle ? oldStyle->shapeMargin() :
RenderStyle::initialShapeMargin();

The const don’t add much value here.

> Source/WebCore/rendering/RenderBox.cpp:405
>	   ShapeOutsideInfo* shapeOutsideInfo =
ShapeOutsideInfo::ensureInfo(this);
>	   shapeOutsideInfo->dirtyShapeSize();

This would read better without the local variable.

Also, is it really necessary to create the info with an “ensure” function just
to dirty the size? Seems that if there was no info there is nothing to dirty.

> Source/WebCore/rendering/RenderBox.h:645
> -    void updateShapeOutsideInfoAfterStyleChange(const ShapeValue*
shapeOutside, const ShapeValue* oldShapeOutside);
> +    void updateShapeOutsideInfoAfterStyleChange(const RenderStyle*, const
RenderStyle* oldStyle);

First argument should be const RenderStyle& since it’s guaranteed to never be
null.

> LayoutTests/ChangeLog:733
> +	   [CSS Shapes] Basic shapes should be animatable for shape-outside
> +	   https://bugs.webkit.org/show_bug.cgi?id=122343
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Add tests checking that shape-outside basic shape values correctly
tween
> +	   between values.
> +
> +	   * animations/resources/animation-test-helpers.js:
> +	   (getPropertyValue): Add shape-outside to list of properties that do
not parse
> +	   with the default behavior.
> +	   (comparePropertyValue): Compare shape-outsides after parsing their
shape notation.
> +	   *
fast/shapes/shape-outside-floats/shape-outside-animation-expected.txt: Added.
> +	   * fast/shapes/shape-outside-floats/shape-outside-animation.html:
Added.

Extra change log here.