| Summary: | dispatchDidFinishDocumentLoad and window.onload order are inconsistent | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Aaron Boodman <aa> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Aaron Boodman <aa> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | slewis | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Aaron Boodman
2008-11-16 20:21:29 PST
Created attachment 25206 [details] Move dispatchDidFinishLoading() to before checkCompleted() This patch was Maciej's recommendation, here: https://lists.webkit.org/pipermail/webkit-dev/2008-November/005723.html Comment on attachment 25206 [details]
Move dispatchDidFinishLoading() to before checkCompleted()
r=me
Reverted http://trac.webkit.org/changeset/38582 due to failing layout tests. Created attachment 25585 [details]
New patch, this time with regression tests updated.
Comment on attachment 25585 [details] New patch, this time with regression tests updated. Looks good. > + https://bugs.webkit.org/show_bug.cgi?id=22301 > + > + Make dispatchDidFinishLoading() always fire before didFinishLoadForFrame(). Tabs in ChangeLog. This makes it harder for the committer to land your patch. > Index: LayoutTests/fast/dom/Window/get-set-properties-expected.txt > =================================================================== > --- LayoutTests/fast/dom/Window/get-set-properties-expected.txt (revision 38838) > +++ LayoutTests/fast/dom/Window/get-set-properties-expected.txt (working copy) > @@ -1,4 +1,3 @@ > -main frame "window.marker.toString()" - has 1 onunload handler(s) > This page tests getting and setting window properties and functions. Why is this test result correct? It looks to me like you broke something if the onunload handler message isn't being logged any more. review- because of this test result change that needs to be investigated. If you can explain why the test result is OK, then it's fine to put this same patch up for review again. Created attachment 25789 [details]
New patch, this time without tabs
Comment on attachment 25789 [details]
New patch, this time without tabs
This is just the original patch without layout test changes. We can't land this unless we update the layout tests to expect the new results. And if you read my previous review comments, at least some of the layout test results seem to reflect a problem with the patch.
I know, I didn't mean to upload that one. Sorry. Working on the correct patch now. Created attachment 25790 [details]
New patch
Sorry, the last patch was incorrect. Also, sorry about the tabs, I am still getting used to working on a mac.
I worried about rebaselining these tests at first but I think I convinced myself it is the right thing to do.
The removed lines from the layout tests were getting printed out by DumpRenderTree's FrameLoaderDelegate implementation inside didFinishDocumentLoadForFrame().
Before my change FrameLoader would sometimes call didFinishDocumentLoadForFrame() after firing window.onload events. Since this layout test is doing its work in window.onload, this would result in some remnants of the test (in this case, the fact that it set a window.onunload hander) showing up in the output. I don't think this is relevant to what these tests are attempting to prove, though.
But I'm new to WebKit so please let me know if I'm wrong.
I added the onunload handle message to verify that WebKit was counting unonload events correctly. I use that count to do fast teardown at quit. If those messages aren't being logged anymore then something in that mechanism may be broken. I will need to take a look at the patch before I can verify that removing those messages is correct. Comment on attachment 25790 [details]
New patch
The unonload changes are fine.
|