Bug 40102

Summary: WebCore EventHandler needs to take account of onLoad events fired before layout() complete
Product: WebKit Reporter: Kent Tamura <tkent>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Normal CC: ap, hausmann, hyatt, kenneth, laszlo.gombos, ossy, robert, thakis
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 39918    
Attachments:
Description Flags
Patch
none
Patch kenneth: review+

Description Kent Tamura 2010-06-02 21:25:59 PDT
editing/input/page-up-down-scrolls.html, which was introduced by r60591, fails on Qt Linux.
Comment 1 Nico Weber 2010-06-02 22:27:46 PDT
Does webkit/qt/linux have sendEvent in its DRT? Are there instructions on how to build webkit/qt/linux somewhere? How is it normally handled when a new test fails only on a subset of ports?
Comment 2 Csaba Osztrogonác 2010-06-03 01:08:10 PDT
pretty diff can be found here:
http://build.webkit.org/results/Qt%20Linux%20Release/r60591%20%2812729%29/editing/input/page-up-down-scrolls-pretty-diff.html

Yes, Qt-DRT has sendEvent, pageUp and pageDown should be handled.

This test fails only on Qt port, so it was a correct solution.
We usually file a bug and put the test into the skipped list 
as you did. And then somebody will try to fix the bug.

(If you would like to build QtWebKit on Linux, try to follow http://trac.webkit.org/wiki/BuildingQtOnLinux . But I'm not
sure if it is an absolutely perfect and up-to-date documentation.)
Comment 3 Csaba Osztrogonác 2010-06-05 02:04:49 PDT
Simon, what do you think, is it a release blocker?

Or should we simple cherry-pick http://trac.webkit.org/changeset/60600
which add editing/input/page-up-down-scrolls.html to the Skipped list?
Comment 4 Laszlo Gombos 2010-06-08 11:04:13 PDT
I do not think this is a release blocker - keeping it with P2 priority.
Comment 5 Csaba Osztrogonác 2010-06-09 06:51:17 PDT
(In reply to comment #4)
> I do not think this is a release blocker - keeping it with P2 priority.
OK, but if it isn't a release blocker, we should cherry-pick 
http://trac.webkit.org/changeset/60600 to make buildbot happier.

Simon, could you do it?
Comment 6 Robert Hogan 2010-06-13 04:52:45 PDT
This is failing because WebCore 'cheats' by firing the onLoad event before the FrameView has performed its final layout(). This is deliberate on the part of WebCore, from implicitClose() in Document.cpp (1862):

    // Make sure both the initial layout and reflow happen after the onload
    // fires. This will improve onload scores, and other browsers do it.
    // If they wanna cheat, we can too. -dwh

So at first glance you'd think Qt needs to ensure that it forces the layout before acting on onLoad events that need to know the correct layout, which is presumably what other ports do.

In the case of this test, QWebPagePrivate::handleScrolling() could always check that the layout is final before doing anything - but that's just one possible situation.

Qt doesn't know it's acting on an onLoad event so there doesn't appear to be a way of doing this in a generic that way will cover all cases.

This also seems to me like something WebCore should be looking after anyway - since it's the one that's 'cheating'. 

That said, I've no idea why this only fails for Qt - maybe it's something to do the way it queues events.
Comment 7 Robert Hogan 2010-06-13 05:04:08 PDT
Created attachment 58591 [details]
Patch
Comment 8 Robert Hogan 2010-06-13 05:07:45 PDT
(In reply to comment #7)
> Created an attachment (id=58591) [details]
> Patch

This patch makes WebCore take account of it's own 'cheating' on the firing of eventLoad handlers. I think this is the correct thing to do, it seems unfair to make ports worry about whether the layout is ready when processing onLoad events.
Comment 9 Kenneth Rohde Christiansen 2010-06-13 05:50:03 PDT
It is a bit confusing that the patch says [Qt] and only touches WebCore code.
Comment 10 Kenneth Rohde Christiansen 2010-06-13 05:51:38 PDT
(In reply to comment #9)
> It is a bit confusing that the patch says [Qt] and only touches WebCore code.

I was commenting on the WebCore changelog:

 5         [Qt] editing/input/page-up-down-scrolls.html failure
 6 
 7         https://bugs.webkit.org/show_bug.cgi?id=40102

Shouldn't this be

WebCore EventHandler needs to take account of onLoad events fired before layout() complete
https://bugs.webkit.org/show_bug.cgi?id=40102

instead?
Comment 11 Robert Hogan 2010-06-13 06:18:18 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > It is a bit confusing that the patch says [Qt] and only touches WebCore code.
> 
> I was commenting on the WebCore changelog:
> 
>  5         [Qt] editing/input/page-up-down-scrolls.html failure
>  6 
>  7         https://bugs.webkit.org/show_bug.cgi?id=40102
> 
> Shouldn't this be
> 
> WebCore EventHandler needs to take account of onLoad events fired before layout() complete
> https://bugs.webkit.org/show_bug.cgi?id=40102
> 
> instead?

I changed the bug title after uploading the patch!
Comment 12 Robert Hogan 2010-06-14 14:48:22 PDT
Created attachment 58704 [details]
Patch
Comment 13 Kenneth Rohde Christiansen 2010-06-14 17:56:02 PDT
Comment on attachment 58704 [details]
Patch

Seems OK with me.
Comment 14 Simon Hausmann 2010-06-15 06:59:25 PDT
This should be included in the release branch, as r60591 is also included and the test indeed fails on the release branch at the moment.
Comment 15 Robert Hogan 2010-06-17 15:20:18 PDT
Committed r61353: <http://trac.webkit.org/changeset/61353>
Comment 16 Alexey Proskuryakov 2010-06-17 17:02:34 PDT
>        WebCore 'cheats' by firing onLoad events before the frame's layout

Why are you saying so? Do we violate any specification?
Comment 17 Kenneth Rohde Christiansen 2010-06-17 17:08:43 PDT
(In reply to comment #16)
> >        WebCore 'cheats' by firing onLoad events before the frame's layout
> 
> Why are you saying so? Do we violate any specification?

Please read comment #6, it explains it
Comment 18 Alexey Proskuryakov 2010-06-17 17:14:33 PDT
Thanks, that explains why ChangeLog said so. But it's not clear to me if this ancient comment is accurate.
Comment 19 Simon Hausmann 2010-06-18 06:01:47 PDT
Revision r61353 cherry-picked into qtwebkit-2.0 with commit e32cb21d4f1787147bcb681883b96a95f867749a