Full Text Bug Listing

Naoki Takano

Reported 2011-05-01 19:42:09 PDT

Automated Bug 5768 test from manual test.

Naoki Takano

Comment 1 2011-05-01 19:45:15 PDT

Created attachment 91875 [details] Patch

Naoki Takano

Comment 2 2011-05-01 19:48:57 PDT

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,

Naoki Takano

Comment 3 2011-05-01 19:49:34 PDT

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,

Adam Barth

Comment 4 2011-05-01 19:58:40 PDT

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.

Naoki Takano

Comment 5 2011-05-01 23:48:48 PDT

Created attachment 91887 [details] Patch

Naoki Takano

Comment 6 2011-05-01 23:50:42 PDT

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,

Ryosuke Niwa

Comment 7 2011-05-02 11:46:56 PDT

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 > + 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.

Ryosuke Niwa

Comment 8 2011-05-02 11:47:16 PDT

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 > + 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.

Naoki Takano

Comment 9 2011-05-02 12:05:57 PDT

Thank you for your review every time. (In reply to comment #8) > > LayoutTests/fast/frames/frame-with-noresize-refresh.html:3 > > + > > 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.

Ryosuke Niwa

Comment 10 2011-05-02 12:24:30 PDT

(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 > > > + > > > > 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.

Naoki Takano

Comment 11 2011-05-02 18:45:24 PDT

Created attachment 92032 [details] Patch

Naoki Takano

Comment 12 2011-05-02 18:45:46 PDT

Comment on attachment 92032 [details] Patch Please review again.

Ryosuke Niwa

Comment 13 2011-05-03 09:16:45 PDT

(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?

Naoki Takano

Comment 14 2011-05-03 09:50:08 PDT

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?

Naoki Takano

Comment 15 2011-05-03 09:51:18 PDT

> 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?

Ryosuke Niwa

Comment 16 2011-05-03 09:52:16 PDT

(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?

Naoki Takano

Comment 17 2011-05-03 09:59:27 PDT

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?

Ryosuke Niwa

Comment 18 2011-05-03 10:14:17 PDT

(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.

Naoki Takano

Comment 19 2011-05-03 10:20:36 PDT

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.

Daniel Bates

Comment 20 2011-05-06 12:23:37 PDT

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.

Naoki Takano

Comment 21 2011-05-06 13:05:05 PDT

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.

Attachments
Patch (3.66 KB, patch) 2011-05-01 19:45 PDT, Naoki Takano	no flags	Details Formatted Diff Diff
Patch (3.64 KB, patch) 2011-05-01 23:48 PDT, Naoki Takano	no flags	Details Formatted Diff Diff
Patch (3.61 KB, patch) 2011-05-02 18:45 PDT, Naoki Takano	dbates: review- dbates: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Bug 59912