Bug 22301 – dispatchDidFinishDocumentLoad and window.onload order are inconsistent

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Aaron Boodman]

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.

[Aaron Boodman]

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

[Maciej Stachowiak]

Comment on attachment 25206 [details]
Move dispatchDidFinishLoading() to before checkCompleted()

r=me

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/38574

[Darin Fisher (:fishd, Google)]

Reverted http://trac.webkit.org/changeset/38582 due to failing layout tests.

[Aaron Boodman]

Created attachment 25585 [details]
New patch, this time with regression tests updated.

[Darin Adler]

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.

[Aaron Boodman]

Created attachment 25789 [details]
New patch, this time without tabs

[Darin Adler]

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.

[Aaron Boodman]

I know, I didn't mean to upload that one. Sorry. Working on the correct patch now.

[Aaron Boodman]

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.

[Stephanie Lewis]

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.

[Stephanie Lewis]

Comment on attachment 25790 [details]
New patch

The unonload changes are fine.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/39097