Bug 38179 - Due to ScrollView, QWebPage::setViewportSize() forces two layout of the render tree
Summary: Due to ScrollView, QWebPage::setViewportSize() forces two layout of the rende...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: Performance, Qt, QtTriaged
: 34501 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-04-27 00:49 PDT by Benjamin Poulain
Modified: 2013-12-23 19:22 PST (History)
11 users (show)

See Also:


Attachments
Remove superfluous forceLayout() call (1.14 KB, patch)
2010-04-27 03:47 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Shark trace showing three calls to WebCore::RenderView:layout (1.64 KB, text/plain)
2010-04-29 07:33 PDT, Nate Whetsell
no flags Details
Temporary patch (1.46 KB, patch)
2010-05-12 21:39 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (2.89 KB, patch)
2010-05-20 12:31 PDT, Benjamin Poulain
hyatt: review-
hyatt: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2010-04-27 00:49:42 PDT
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
Comment 1 Andreas Kling 2010-04-27 03:47:48 PDT
Created attachment 54407 [details]
Remove superfluous forceLayout() call
Comment 2 Benjamin Poulain 2010-04-27 04:15:24 PDT
*** Bug 34501 has been marked as a duplicate of this bug. ***
Comment 3 Simon Hausmann 2010-04-28 12:06:06 PDT
Comment on attachment 54407 [details]
Remove superfluous forceLayout() call

Good catch!
Comment 4 Kenneth Rohde Christiansen 2010-04-28 12:27:58 PDT
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.
Comment 5 Benjamin Poulain 2010-04-29 00:01:43 PDT
(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.
Comment 6 Simon Hausmann 2010-04-29 03:43:35 PDT
Committed r58497: <http://trac.webkit.org/changeset/58497>
Comment 7 Simon Hausmann 2010-04-29 04:05:31 PDT
Revision r58497 cherry-picked into qtwebkit-2.0 with commit 1bfff59ac27f9382f6c11f18144bf129bcc9f1a4
Comment 8 Benjamin Poulain 2010-04-29 04:23:17 PDT
Reopening, more patches should follow.
Comment 9 Nate Whetsell 2010-04-29 07:33:16 PDT
Created attachment 54705 [details]
Shark trace showing three calls to WebCore::RenderView:layout
Comment 10 Nate Whetsell 2010-04-29 07:34:29 PDT
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.
Comment 11 Kenneth Rohde Christiansen 2010-05-10 19:01:14 PDT
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.
Comment 12 Nate Whetsell 2010-05-10 19:14:13 PDT
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.
Comment 13 Antonio Gomes 2010-05-10 21:50:48 PDT
(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()
Comment 14 Antonio Gomes 2010-05-10 21:57:15 PDT
(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.
Comment 15 Benjamin Poulain 2010-05-12 21:34:52 PDT
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.
Comment 16 Benjamin Poulain 2010-05-12 21:39:41 PDT
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
Comment 17 Eric Seidel (no email) 2010-05-17 00:48:01 PDT
Unclear what the status of this patch is.  Is it to be landed?
Comment 18 Andreas Kling 2010-05-17 01:27:41 PDT
(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.
Comment 19 Benjamin Poulain 2010-05-20 12:31:56 PDT
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.
Comment 20 Benjamin Poulain 2010-05-20 12:33:58 PDT
> 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 :)
Comment 21 Kenneth Rohde Christiansen 2010-05-20 12:58:08 PDT
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.
Comment 22 Kenneth Rohde Christiansen 2010-05-22 12:22:21 PDT
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.
Comment 23 Simon Hausmann 2010-06-16 23:31:21 PDT
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?
Comment 24 Benjamin Poulain 2010-06-18 08:50:14 PDT
(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).
Comment 25 Dave Hyatt 2010-06-21 12:09:02 PDT
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).
Comment 26 Antonio Gomes 2010-09-19 13:32:32 PDT
Benjamin, planning on follow up here?
Comment 27 Benjamin Poulain 2010-09-19 13:52:40 PDT
(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.