[webkit-reviews] review denied: [Bug 59912] Automated Bug 5768 test from manual test. : [Attachment 91887] Patch

Mon May 2 11:46:56 PDT 2011

Ryosuke Niwa <rniwa at webkit.org> has denied Naoki Takano
<takano.naoki at gmail.com>'s request for review:
Bug 59912: Automated Bug 5768 test from manual test.
https://bugs.webkit.org/show_bug.cgi?id=59912

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=91887&action=review

r- due to various nits.

> LayoutTests/fast/frames/frame-with-noresize-refresh.html:3
> +<!--This test checks if multiple frame loading correctly refreshes layout
with noresize attribute in frame.-->

It's more helpful to put this in body so that readers can see what the test is
testing in the expected result.

> LayoutTests/fast/frames/frame-with-noresize-refresh.html:31
> +    switch(i) {
> +	   case 1:
> +	       f.src = "resources/frame2.html";
> +	       break;
> +	   case 2:
> +	       f.src = "resources/frame1.html";
> +	       break;
> +	   case 3:
> +	       if (window.layoutTestController)
> +		   layoutTestController.notifyDone();
> +	       break;
> +    }

Do we really need this switch statement?  Can't we just do something along the
line of:
if (!is_frame2 && !called_from_frame2)
    f.src = "resources/frame2.html";
else if (is_frame2)
    f.src = "resources/frame1.html";
else {
    if (window.layoutTestController)
    layoutTestController.notifyDone();
}

> LayoutTests/fast/frames/resources/frame1.html:1
> +<html>

No DOCTYPE

> LayoutTests/fast/frames/resources/frame1.html:4
> +<head>
> +<title>frame1</title>
> +</head>

Do we need title?

> LayoutTests/fast/frames/resources/frame1.html:7
> +Frame 1.
> +<div id="msg">First loading. FAIL if you see this message as the final
message.</div>

Frame 1 and first loading are redundant.  You should put either but not both.

> LayoutTests/fast/frames/resources/frame1.html:9
> +  if(parent.parent.parent.test(false))

Why 3 parents??

> LayoutTests/fast/frames/resources/frame1.html:10
> +    document.getElementById('msg').innerText = "SUCCESS to load twice.";

I don't think you need to say " to load twice".  PASS or SUCCESS suffice.