Bug 38179

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

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

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

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.

(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.

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

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

Reopening, more patches should follow.

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

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.

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.

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.

(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()

(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.

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.

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

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

(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.

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.

> 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 :)

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.

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.

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?

(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 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).

Benjamin, planning on follow up here?

(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.