Bug 38179 – Due to ScrollView, QWebPage::setViewportSize() forces two layout of the render tree

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Benjamin Poulain]

QWebPage::setViewportSize() layout the render tree multiple times.

The call to setFrameRect() will trigger a full re-layout if the size have changed. Later in the function, a second full layout is done because we call view->forceLayout().

See the comments of Nate Whetsell on http://bugreports.qt.nokia.com/browse/QTBUG-5929

[Andreas Kling]

Created attachment 54407 [details]
Remove superfluous forceLayout() call

[Benjamin Poulain]

*** Bug 34501 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Comment on attachment 54407 [details]
Remove superfluous forceLayout() call

Good catch!

[Kenneth Rohde Christiansen]

Anyway it shouldn't really make any difference. forceLayout is just a normal layout, and it will only layout if something is marked as needsLayout.

[Benjamin Poulain]

(In reply to comment #4)
> Anyway it shouldn't really make any difference. forceLayout is just a normal
> layout, and it will only layout if something is marked as needsLayout.

It does layout, the layout is invalidated earlier for the scrollbar. Nate Whetsell have other comments, we really layout 3 times.

Andreas is working on follow up patches for the last superfluous layout.

[Simon Hausmann]

Committed r58497: <http://trac.webkit.org/changeset/58497>

[Simon Hausmann]

Revision r58497 cherry-picked into qtwebkit-2.0 with commit 1bfff59ac27f9382f6c11f18144bf129bcc9f1a4

[Benjamin Poulain]

Reopening, more patches should follow.

[Nate Whetsell]

Created attachment 54705 [details]
Shark trace showing three calls to WebCore::RenderView:layout

[Nate Whetsell]

Hi all,

Benjamin suggested I move over here from the Qt JIRA site.

I think that there is one more superfluous layout occurring. I'll try to distill what I think is causing this from my comments at http://bugreports.qt.nokia.com/browse/QTBUG-5929.

In WebCore::ScrollView::updateScrollbars, there are two calls to WebCore::FrameView::visibleContentsResized. The second call (currently line 414 of http://trac.webkit.org/browser/trunk/WebCore/platform/ScrollView.cpp) is immediately preceded by a call to WebCore::FrameView::contentsResized. I think this means that needsLayout is set to true before visibleContentsResized is called, which guarantees that a layout will occur here.

The call to WebCore:: FrameView:: visibleContentsResized eventually leads to WebCore::ScrollView::setContentsSize. In this function, WebCore::ScrollView::updateScrollbars is called again. If, in this second call to WebCore::ScrollView::updateScrollbars, execution gets to line 414, I believe there will be another layout. (The m_updateScrollbarsPass < cMaxUpdateScrollbarsPass test at line 411 prevents additional layouts from occurring.)

For ease of reference, I've attached a report (shark.txt) generated from a Shark trace I did to try and pin down this issue. The trace shows execution time divided evenly between three calls to WebCore::RenderView::layout, and two calls to WebCore::ScrollView::updateScrollbars.

[Kenneth Rohde Christiansen]

Nate, are you still looking into this? If not, we need to figure out whether to postpone it or get someone else to look at it.

[Nate Whetsell]

Hi Kenneth,

I didn't realize I was the only one looking at this. My comments here and at http://bugreports.qt.nokia.com/browse/QTBUG-5929 are unfortunately all I'm able to provide. I don't have enough experience with Qt and Webkit to take this further.

[Antonio Gomes]

(In reply to comment #6)
> Committed r58497: <http://trac.webkit.org/changeset/58497>

this change (believe it or not) broke tst_qgraphicswebview.cpp @ QGraphicsWebView::crashOnViewlessWebPages()

[Antonio Gomes]

(In reply to comment #13)
> (In reply to comment #6)
> > Committed r58497: <http://trac.webkit.org/changeset/58497>
> 
> this change (believe it or not) broke tst_qgraphicswebview.cpp @ QGraphicsWebView::crashOnViewlessWebPages()

or to  say it better, this bug might had revealed a real bug.

[Benjamin Poulain]

There is still 3 calls to FrameView::layout() going on when resizing to an inferior size.

In updateScrollbars:
1) When resizing to an inferior size, we always assume the scrollbars will be set: "newHasHorizontalScrollbar = docSize.width() > visibleWidth();". And we layout with this setting.
In FrameView::layout():
2) We now know we have enough space without the scrollbar, so we disable them and layout() again
3) contentsResized(); is called after updateScrollbars(). It mark the layout as dirty and we will call layout one more time as soon as we enter a function that the layout to be done.

The calls (1) and (2) do a full relayout of the page.

[Benjamin Poulain]

Created attachment 55939 [details]
Temporary patch

This patch get rid of the 2 superfluous calls to layout().

Not ready yet because:
-I need to make sure the case (!newHasHorizontalScrollbar && hasHorizontalScrollbar && vScroll != ScrollbarAlwaysOn) will not fail because of cMaxUpdateScrollbarsPass == 2
-I need to understand why the call inside ScrollView::setFrameRect() were in reverse order
-I am way too tired to be confident in the patch

[Eric Seidel (no email)]

Unclear what the status of this patch is.  Is it to be landed?

[Andreas Kling]

(In reply to comment #17)
> Unclear what the status of this patch is.  Is it to be landed?

Benjamin will finalize it when he returns to the office on Wednesday.

[Benjamin Poulain]

Created attachment 56621 [details]
Patch

The real patch. From my previous comments:

> -I need to make sure the case (!newHasHorizontalScrollbar && hasHorizontalScrollbar && vScroll != ScrollbarAlwaysOn) will not fail because of cMaxUpdateScrollbarsPass == 2
This is not a problem in the updated version because I only care about the case when newHasHorizontalScrollbar is true.
The other case does not need change since the behavior is already optimal.

> -I need to understand why the call inside ScrollView::setFrameRect() were in reverse order
Unfortunately, there is no history for that. This code comes from a big merge from the Windows port of WebKit.

[Benjamin Poulain]

> The real patch.

Also, the patch does not include unit tests because I am not aware of a way to count the number of re-layout() of a page from JavaScript.

If there is a way, I would be happy to make a test case :)

[Kenneth Rohde Christiansen]

You could expose that yourself. It is very common to add such hooks when adding layout tests, given that they don't introduce a performance penalty.

[Kenneth Rohde Christiansen]

Changing the title, as the patch only affects WebCore code.

Hyatt, this is scrollbar related, can you take a look if you have time? Thanks.

[Simon Hausmann]

AFAICS this is a bug and it's an annoying one. At the same time it's not a regression or a crash, but it's blocking the release. I suggest to move it to 39313 - Benjamin/Andreas, what do you think?

[Benjamin Poulain]

(In reply to comment #23)
> AFAICS this is a bug and it's an annoying one. At the same time it's not a regression or a crash, but it's blocking the release. I suggest to move it to 39313 - Benjamin/Andreas, what do you think?

Yep, that is a good idea. I did not know about 39313.

No big deal if this is only integrated for WebKit 2.1, it is just a performance boost for a corner case (resizing down).

[Dave Hyatt]

Comment on attachment 56621 [details]
Patch

I'm not convinced these changes are right, especially the move of updateScrollbars to after contentsResized. You need to patch WebDynamicScrollbarsView.m on Mac as well so we keep the logic in sync. I'd also recommend testing on Mac with the changes in place, since you may catch bugs with these changes when running layout tests there (since it has more test coverage).

[Antonio Gomes]

Benjamin, planning on follow up here?

[Benjamin Poulain]

(In reply to comment #26)
> Benjamin, planning on follow up here?

I would love to update, but I am short on time unfortunately, and this might require a full day of work.