Bug 22301 - dispatchDidFinishDocumentLoad and window.onload order are inconsistent
Summary: dispatchDidFinishDocumentLoad and window.onload order are inconsistent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Aaron Boodman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-16 20:21 PST by Aaron Boodman
Modified: 2008-12-08 10:14 PST (History)
1 user (show)

See Also:


Attachments
Move dispatchDidFinishLoading() to before checkCompleted() (1.30 KB, patch)
2008-11-16 20:33 PST, Aaron Boodman
no flags Details | Formatted Diff | Diff
New patch, this time with regression tests updated. (10.45 KB, patch)
2008-11-28 23:24 PST, Aaron Boodman
darin: review-
Details | Formatted Diff | Diff
New patch, this time without tabs (1.30 KB, patch)
2008-12-05 14:17 PST, Aaron Boodman
darin: review-
Details | Formatted Diff | Diff
New patch (10.42 KB, patch)
2008-12-05 14:42 PST, Aaron Boodman
slewis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2008-11-16 20:21:29 PST
dispatchDidFinishDocumentLoad usually fires before window.onload, but can sometimes fire after. It would be better if it were consistent. And since we already have a hook for when the window is completley loaded in dispatchDidFinishLoading, I think it should always fire before.
Comment 1 Aaron Boodman 2008-11-16 20:33:10 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 2 Maciej Stachowiak 2008-11-17 19:15:44 PST
Comment on attachment 25206 [details]
Move dispatchDidFinishLoading() to before checkCompleted()

r=me
Comment 3 Darin Fisher (:fishd, Google) 2008-11-18 13:22:03 PST
http://trac.webkit.org/changeset/38574
Comment 4 Darin Fisher (:fishd, Google) 2008-11-18 15:54:51 PST
Reverted http://trac.webkit.org/changeset/38582 due to failing layout tests.
Comment 5 Aaron Boodman 2008-11-28 23:24:28 PST
Created attachment 25585 [details]
New patch, this time with regression tests updated.
Comment 6 Darin Adler 2008-12-05 09:54:24 PST
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.
Comment 7 Aaron Boodman 2008-12-05 14:17:49 PST
Created attachment 25789 [details]
New patch, this time without tabs
Comment 8 Darin Adler 2008-12-05 14:21:25 PST
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.
Comment 9 Aaron Boodman 2008-12-05 14:36:33 PST
I know, I didn't mean to upload that one. Sorry. Working on the correct patch now.
Comment 10 Aaron Boodman 2008-12-05 14:42:41 PST
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.
Comment 11 Stephanie Lewis 2008-12-05 17:39:34 PST
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 12 Stephanie Lewis 2008-12-05 17:59:13 PST
Comment on attachment 25790 [details]
New patch

The unonload changes are fine.
Comment 13 Darin Fisher (:fishd, Google) 2008-12-08 10:14:45 PST
http://trac.webkit.org/changeset/39097