Automated Bug 5768 test from manual test.
Created attachment 91875 [details] Patch
Comment on attachment 91875 [details] Patch The test is originally written by Darin at Bug 5768, but, at the time, it was not automatically run test. So I change the test, please review my change. Thanks,
BTW, this is originally requested in webkit-meeting Hack-a-thon. (In reply to comment #2) > (From update of attachment 91875 [details]) > The test is originally written by Darin at Bug 5768, but, at the time, it was not automatically run test. > > So I change the test, please review my change. > > Thanks,
Comment on attachment 91875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=91875&action=review > LayoutTests/fast/frames/frame-with-noresize-refresh.html:12 > +if (window.layoutTestController) > + layoutTestController.dumpAsText(); > + > +if (window.layoutTestController) > + layoutTestController.dumpChildFramesAsText(); > + > +if (window.layoutTestController) > + layoutTestController.waitUntilDone(); This can all be combined into one if-block. > LayoutTests/fast/frames/frame-with-noresize-refresh.html:14 > +alert("This test checks if multiple frame loading correctly refreshes layout with noresize attribute in frame."); I'd just make this a comment instead of an alert. An alert can be annoying when running the test manually. > LayoutTests/fast/frames/frame-with-noresize-refresh.html:23 > + window.setTimeout(test, 100); Why these set timeouts? They look like race conditions to me.
Created attachment 91887 [details] Patch
Comment on attachment 91887 [details] Patch Adam, Could you please review again? I removed settimeout functions and used call back from frame source files. Actually settimeout functions are used from original version, but I suppose my version would be better;-) Thanks,
Comment on attachment 91887 [details] Patch 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.
Thank you for your review every time. (In reply to comment #8) > > 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. Actually, we cannot use <body> tag with <frameset> at the same time. Or should I move the message into frame1.html in <body>? > 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(); > } Yes, you are right. This is much clearer than switch version. > > LayoutTests/fast/frames/resources/frame1.html:4 > > +<head> > > +<title>frame1</title> > > +</head> > > Do we need title? No, I can remove them. > > 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. I will remain "Frame 1." > > LayoutTests/fast/frames/resources/frame1.html:9 > > + if(parent.parent.parent.test(false)) > > Why 3 parents?? We want to access to the test() function in frame-with-noresize-refresh.html. The first parent is <frame> in frame-with-noresize-refresh.html and the second is <frameset> and the third is <body>. So we need 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. Sure.
(In reply to comment #9) > Thank you for your review every time. > > (In reply to comment #8) > > > 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. > > > Actually, we cannot use <body> tag with <frameset> at the same time. > Or should I move the message into frame1.html in <body>? Oops, I didn't see frameset. You could use console.log() then.
Created attachment 92032 [details] Patch
Comment on attachment 92032 [details] Patch Please review again.
(In reply to comment #2) > (From update of attachment 91875 [details]) > The test is originally written by Darin at Bug 5768, but, at the time, it was not automatically run test. > > So I change the test, please review my change. I don't see any test attached on the bug 5768. Where did you get the test? If we're converting a manual test, we should be removing the original manual test. Also, did you make sure the bug 5768 reproduces with this test when the patch is reverted?
Please click in bug 5768 "unsuccesful attempt at automatic testing" link and you can download zip file which include the test files. I'll remove the manual test. It is really old patch...5 years ago. So the code part which should be applied with the patch. So it is really hard to revert and make the test fail intentionally... (In reply to comment #13) > (In reply to comment #2) > > (From update of attachment 91875 [details] [details]) > > The test is originally written by Darin at Bug 5768, but, at the time, it was not automatically run test. > > > > So I change the test, please review my change. > > I don't see any test attached on the bug 5768. Where did you get the test? If we're converting a manual test, we should be removing the original manual test. Also, did you make sure the bug 5768 reproduces with this test when the patch is reverted?
> I'll remove the manual test. Oops, so the test is not landed into the current repo. It looks like Darin didn't land it because he couldn't run the test with DumpRenderTree at the time. (In reply to comment #14) > Please click in bug 5768 > "unsuccesful attempt at automatic testing" > link and you can download zip file which include the test files. > > I'll remove the manual test. > > It is really old patch...5 years ago. > > So the code part which should be applied with the patch. So it is really hard to revert and make the test fail intentionally... > > (In reply to comment #13) > > (In reply to comment #2) > > > (From update of attachment 91875 [details] [details] [details]) > > > The test is originally written by Darin at Bug 5768, but, at the time, it was not automatically run test. > > > > > > So I change the test, please review my change. > > > > I don't see any test attached on the bug 5768. Where did you get the test? If we're converting a manual test, we should be removing the original manual test. Also, did you make sure the bug 5768 reproduces with this test when the patch is reverted?
(In reply to comment #14) > Please click in bug 5768 > "unsuccesful attempt at automatic testing" > link and you can download zip file which include the test files. That's written by mitz, not darin. > So the code part which should be applied with the patch. So it is really hard to revert and make the test fail intentionally... Then how do we know that your test is testing what it claims to test?
According to Bug 5768, the problem is twice frame loading with noresize attribute. Of course, we don't have to include the bug, we shouldn't land it. But I think the test pattern itself is reasonable to check if multiple time frame loading is working or not. And, as I wrote here, this is originally pointed out to convert by dbates@ at last week webkit meeting. What do you think? (In reply to comment #16) > (In reply to comment #14) > > Please click in bug 5768 > > "unsuccesful attempt at automatic testing" > > link and you can download zip file which include the test files. > > That's written by mitz, not darin. > > > So the code part which should be applied with the patch. So it is really hard to revert and make the test fail intentionally... > > Then how do we know that your test is testing what it claims to test?
(In reply to comment #17) > According to Bug 5768, the problem is twice frame loading with noresize attribute. > > Of course, we don't have to include the bug, we shouldn't land it. I don't really understand what you mean but I don't think we should be speculatively adding a new test.
I mean, if the test is not needed, we don't have to land it. I don't have final judgement;-) Ok, I just leave it. But we might need to make sure dbates@ who originally ask us to make the test run automatically. (In reply to comment #18) > (In reply to comment #17) > > According to Bug 5768, the problem is twice frame loading with noresize attribute. > > > > Of course, we don't have to include the bug, we shouldn't land it. > > I don't really understand what you mean but I don't think we should be speculatively adding a new test.
Comment on attachment 92032 [details] Patch This test seems better suited as a pixel-test/repainting-test instead of a text-only test because the description for bug #5768 (https://bugs.webkit.org/show_bug.cgi?id=5768#c0), in particular the last two sentences, describes a painting/repainting issue. This also coincides with the changes described in the patch for the bug (and additional bug comments) and the omission of dumpAsText() from the manual test case that were landed in changeset 11648, <http://trac.webkit.org/changeset/11648>. I am hesitant to land a test that differs much from the original test case since we are working with limited information due to the amount of time that has elapsed since bug #5768 was fixed.
David, Thank you for your comment. I'm not an expert for DumpRenderTree(), so I leave the problem. If I have time, I'll investigate more. (In reply to comment #20) > (From update of attachment 92032 [details]) > This test seems better suited as a pixel-test/repainting-test instead of a text-only test because the description for bug #5768 (https://bugs.webkit.org/show_bug.cgi?id=5768#c0), in particular the last two sentences, describes a painting/repainting issue. This also coincides with the changes described in the patch for the bug (and additional bug comments) and the omission of dumpAsText() from the manual test case that were landed in changeset 11648, <http://trac.webkit.org/changeset/11648>. > > I am hesitant to land a test that differs much from the original test case since we are working with limited information due to the amount of time that has elapsed since bug #5768 was fixed.