[webkit-reviews] review granted: [Bug 74049] Enable animations of CSS images using -webkit-cross-fade : [Attachment 118315] initial patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 74049: Enable animations of CSS images using -webkit-cross-fade
https://bugs.webkit.org/show_bug.cgi?id=74049

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=118315&action=review


r=me but some transition tests wouldn't hurt.

> LayoutTests/animations/cross-fade-background-image.html:37
> +    @-webkit-keyframes "anim" {
> +	   from { background-image: url(resources/blue-100.png); }
> +	   to	{ background-image: url(resources/green-100.png); }
> +    }
> +    @-webkit-keyframes "animShorthand" {
> +	   from { background: url(resources/blue-100.png); }
> +	   to	{ background: url(resources/green-100.png); }

Animation names are IDENT, not STRING, so should not be quoted.

> LayoutTests/animations/resources/animation-test-helpers.js:54
> +	var matches =
s.match("-webkit-cross-fade\\((.*)\\s*,\\s*(.*)\\s*,\\s*(.*)\\)");

Is the double escaping necessary?

> Source/WebCore/css/CSSCrossfadeValue.cpp:112
> +    if (fromImageSize.width() == toImageSize.width() &&
fromImageSize.height() == toImageSize.height())

Doesn't IntSize have an operator== ?

> Source/WebCore/page/animation/AnimationBase.cpp:239
> +    if (progress == 1.)

1. -> 1 or 1.0

> Source/WebCore/page/animation/AnimationBase.cpp:251
> +    RefPtr<StyleGeneratedImage> generatedImage =
StyleGeneratedImage::create(crossfadeValue.get());

Could just return StyleGeneratedImage::create(crossfadeValue.get());

> Source/WebCore/page/animation/AnimationBase.cpp:274
> +    // FIXME: Support transitioning between NinePieceImages that differ by
more than image content.

Please file a bug for this.

> Source/WebCore/page/animation/AnimationBase.cpp:286
> +    return newNinePieceImage;

return NinePieceImage(....) ?