[webkit-reviews] review denied: [Bug 61338] Element not fully repainted after application and removal of transform : [Attachment 119602] Attaching the test case for element not fully repainted after application and removal of transform

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Vamshi Krishna N
<vnampally at innominds.com>'s request for review:
Bug 61338: Element not fully repainted after application and removal of
transform
https://bugs.webkit.org/show_bug.cgi?id=61338

Attachment 119602: Attaching the test case for element not fully repainted
after application and removal of transform 
https://bugs.webkit.org/attachment.cgi?id=119602&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=119602&action=review


The SVN property you set is confusing our tools (click on the EWS purple
bubbles and you will see that).

You need at least one expected result for several reasons (regardless of the
platform you used to generate it): it gives the reviewers some confidence in
what you are testing and for the port maintainer who will have to generate the
result, it gives them a reference.

r- as you should land the full change including this test case + one expected
result. The test case looks fine though.

> LayoutTests/fast/repaint/transform-rotate-remove.html:1
> +

Unneeded empty line (not repeated after).

> LayoutTests/fast/repaint/transform-rotate-remove.html:9
> +    .A {

Please use something meaningful: here it should be .rotated or something
equivalent.

> LayoutTests/fast/repaint/transform-rotate-remove.html:11
> +	 -moz-transform: rotate(50deg);

Great that you are trying to make it work in Mozilla! Don't forget the
unprefixed version that will be used when browsers drop the prefix: -transform:
rotate(50deg);

> LayoutTests/fast/repaint/transform-rotate-remove.html:18
> +    <script src="resources/repaint.js" type="text/javascript"></script>
> +
> +    <script type="text/javascript">

The 'type' attribute is not needed in both <script> tag.

> LayoutTests/fast/repaint/transform-rotate-remove.html:25
> +	     document.getElementById("try").innerHTML = "Rotation Removed";

This text looks unneeded. If the output is wrong, you will see it in your pixel
result.

> LayoutTests/fast/repaint/transform-rotate-remove.html:36
> +	 }, 0);

Do we really need the setTimeout logic? Sometimes tests do some operation at
the end of the page through an inline <script> (in your case rotate()) and the
repaint logic on the 'load' event as you are doing.

> LayoutTests/fast/repaint/transform-rotate-remove.html:48
> +  <p>Test for <a
href="https://bugs.webkit.org/show_bug.cgi?id=61338">https://bugs.webkit.org/sh
ow_bug.cgi?id=61338</a>.Test that element after transform applied and removed
will not clip the element.one should see the element fully painted</p>

Using text makes the test platform-dependent and is not advised unless you need
it for testing. Here it seems like you could skip it. See
http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

> LayoutTests/fast/repaint/transform-rotate-remove.html:51
> +</br>
> +</br>
> +</br>

Do we need all those <br/>? (not that normally they should be written this way
even if it doesn't make a difference due to HTML5 fixing the syntax).

> LayoutTests/fast/repaint/transform-rotate-remove.html:52
> +  <div id='try' style="border:1px solid red; background-color:pink; height:
50px; width:200px">TEST</div>

Red should usually be avoided in an expected output as it has the meaning
'failed'.