[webkit-reviews] review denied: [Bug 57014] New tests for CSS3: 2d-transforms : [Attachment 87336] css3-2d-transforms tests

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied chereddy
<srinivasulu.chereddy at nokia.com>'s request for review:
Bug 57014: New tests for CSS3: 2d-transforms
https://bugs.webkit.org/show_bug.cgi?id=57014

Attachment 87336: css3-2d-transforms tests
https://bugs.webkit.org/attachment.cgi?id=87336&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87336&action=review

This needs a lot of clean up.

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:14
> +/*declared variables for expected values 
> + left difference, top difference, width difference, expected height
difference */

Don't use multiline comments

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:18
> +var exp_left_diff=0; 
> +var exp_top_diff=0;
> +var exp_width_diff=0; 
> +var exp_height_diff=0; 

spaces before and after the =
No underscores. This is against WebKit style.

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:21
> +// Object initialV for initial values and object finalV for values after
transformation

The indention of the following lines is wrong. Also, never use tabs.

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:23
> +	var initialV = new Object();
> +	var finalV = new Object();	

please use self explaining names. What is the V for? use initialValues. Why not
just using intital and final and so on?

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:30
> +	   // var result1 = document.getElementById ("result1");
> +	//     result1.innerHTML="<b>Before:</b> left:"+initialV.left+", top:
"+initialV.top+", width:"+initialV.width+", height:" +initialV.height;	
> +	  // calling applyTransform		 

Why is this commented out?

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:40
> +	//   var result2 = document.getElementById ("result2");
> +	 //   result2.innerHTML="<b>After:</b> left:"+finalV.left+", top:
"+finalV.top+", width:"+finalV.width+", height:" +finalV.height; 

Ditto, Don't check commented out code in.

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:63
> +		/*2 pixels tolerance is given. We are validating difference in
properties length. So 2 pixels positive side and 2 pixels in negative side are
considered.  */ 	  

Just use //

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:89
> +		if (left1==1&&top1==1&&width1==1&&height1==1)		       


This is general code used by more than one test. Please use WebKit style here.

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:101
> +	 var result=result1;

Ditto.

> LayoutTests/css3/css3-2d-transforms/js/auto_functions.js:113
> +	     resultElement.innerHTML="FAIL";
> +	     resultElement.style.background="red";

Ditto

> LayoutTests/css3/css3-2d-transforms/js/logResults.js:3
> +if (window.layoutTestController)
> +	  { layoutTestController.dumpAsText();

put the brace behind the condition. Indention of all lines is wrong. Don't use
tabs

> LayoutTests/css3/css3-2d-transforms/js/logResults.js:8
> +function notify()
> +	{

aline brace and function. Better to put the brace behind the function

> LayoutTests/css3/css3-2d-transforms/js/logResults.js:13
> + function logResults(result1){

space missing. What does result1 mean? take better names please

> LayoutTests/css3/css3-2d-transforms/js/logResults.js:14
> +	 var result=result1;

Why are you creating a new var herer?

> LayoutTests/css3/css3-2d-transforms/js/logResults.js:19
> +	      if (result=="PASS"){
> +	     resultElement.innerHTML="PASS";
> +	     resultElement.style.background="green";

Spaces please

> LayoutTests/css3/css3-2d-transforms/js/logResults.js:26
> +	     resultElement.innerHTML="FAIL";
> +	     resultElement.style.background="red";
> +		  notify();

Ditto.

>
LayoutTests/css3/css3-2d-transforms/tests/2d_transform_animation_3-expected.txt
:1
> +Prev [index] Next

Wrong text coding

> LayoutTests/css3/css3-2d-transforms/tests/2d_transform_animation_3.html:1
> +<!DOCTYPE html>

indention is wrong

>
LayoutTests/css3/css3-2d-transforms/tests/2d_transform_cssMatrix_attributes.htm
l:28
> +//var display= document.getElementById("display");
> +//display.innerHTML="a: "+m1.a +"<br>"+"b: "+m1.b +"<br>"+"c: "+m1.c
+"<br>"+"d: "+m1.d +"<br>"+"e: "+m1.e +"<br>"+"f: "+m1.f;
> +//calling validate() function

no out commented lines. I don't continue here. Just remaining patterns.