[Webkit-unassigned] [Bug 59912] Automated Bug 5768 test from manual test.

Mon May 2 11:46:57 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=59912

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #91887|review?                     |review-
               Flag|                            |

--- Comment #7 from Ryosuke Niwa <rniwa at webkit.org>  2011-05-02 11:46:56 PST ---
(From update of attachment 91887)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.