[webkit-reviews] review granted: [Bug 59149] (Regression) Existing animations are not replaced when filling. : [Attachment 91679] Patch

Fri Apr 29 09:15:46 PDT 2011

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 59149: (Regression) Existing animations are not replaced when filling.
https://bugs.webkit.org/show_bug.cgi?id=59149

Attachment 91679: Patch
https://bugs.webkit.org/attachment.cgi?id=91679&action=review

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

> LayoutTests/animations/replace-filling-transform.html:2
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
> +   "http://www.w3.org/TR/html4/loose.dtd">

Can be just <!DOCTYPE html>

> LayoutTests/animations/replace-filling-transform.html:4
> +<html lang="en">

Kill the lang.

> LayoutTests/animations/replace-filling-transform.html:7
> +    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
> +    <title>Replace a transform animation that is filling</title>

Kill, kill

> LayoutTests/animations/replace-filling-transform.html:8
> +    <style type="text/css" media="screen">

Kill the attributes

> LayoutTests/animations/replace-filling-transform.html:23
> +	      from {
> +		-webkit-transform: translate3d(100px, 0, 0);
> +	      }
> +	      to {
> +		-webkit-transform: translate3d(200px, 0, 0);
> +	      }

If this test is about hardware animation, then it should go into animation/3d

> LayoutTests/animations/replace-filling-transform.html:35
> +    <script type="text/javascript" charset="utf-8">

Kill the attributes

> LayoutTests/animations/replace-filling-transform.html:49
> +	       if (computedValue == expectedValue) {
> +		   result.innerHTML = "PASS - final state was " +
expectedValue;
> +	       } else {
> +		   result.innerHTML = "FAIL - final state was " + computedValue
+ " expected " + expectedValue;
> +	       }

No need for braces on single lines.

> LayoutTests/animations/replace-filling-transform.html:53
> +	       }

Ditto.

> LayoutTests/animations/replace-filling-transform.html:71
> +	       if (window.layoutTestController) {
> +		   layoutTestController.dumpAsText();
> +		   layoutTestController.waitUntilDone();
> +	       }

We normally put this code bare in the <script> tag.