Bug 136433 – REGRESSION (r172832): Poor 2-finger scrolling performance at theverge.com articles (all tiles repaint)

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

Performing a two-finger scroll gesture on theverge.com articles causes all tiles to repaint, and scrolling either doesn’t happen or happens very slowly.

To reproduce, navigate to the URL and two-finger scroll vertically. With repaint counters on, you’ll see that all the tiles repaint with every wheel event (even if the page doesn’t scroll at all).

This was cause by <http://trac.webkit.org/r172832>, the fix for bug 91655.

<rdar://problem/18193942>

[Beth Dakin]

I'm actually having a hard time reproducing the bug at The Verge, but it is very obvious at hulu.com for me.

[Beth Dakin]

Created attachment 237578 [details]
Patch

Unfortunately, I still don't totally understand what Hulu is doing to get into this state, and therefore I don't have a test case. I have failed so far in my attempts to make a reduction.

[Beth Dakin]

Created attachment 237596 [details]
Test case

Here's a test case that reproduces the bug! So far I haven't found a way to turn it into a layout test since the bug does not reproduce when scrolling programatically.

[Tim Horton]

(In reply to comment #4)
> Created an attachment (id=237596) [details]
> Test case
> 
> Here's a test case that reproduces the bug! So far I haven't found a way to turn it into a layout test since the bug does not reproduce when scrolling programatically.

What if you use bfulgham's magical scroll event generating internals thing? Those look way more like real scroll events to WebKit IIRC.

[Beth Dakin]

Created attachment 237607 [details]
Patch

A few notes about this patch. I took Tim's suggestion, and tried to use Brent's momentum events to create a test, and I kind of succeeded. The test seems to work on my machine. I am just dubious because I am not 100% convinced that these momentum events will do the exact same thing on all computers. So I put a note in the test about how if it turns out to be flakey, we can skip it. But! It is effective on my machine.

Another note. Sitting with Simon and Sam about this bug, they also want ScrollableArea::handleWheelEvent() to return early is the ScrollableArea is not scrollable or rubberbandable. I am going to implement that in a followup patch because adding a virtual function to ScrollableArea called isScrollableOrRubberbandable() turns out to be a bit complicated for RenderListBox since there is already a virtual function of that same name in its RenderObject inheritance chain. Ah, the joys of multiple inheritance. Anyway, there are obviously solutions to that problem, but it seems involved enough that it should be a separate patch.

[Beth Dakin]

Comment on attachment 237607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237607&action=review

> LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/root-overflow-with-mousewheel.html:35
> +    setTimeout(checkForScroll, 500);

Oh, I can get rid of this setTimeout completely, so that's good.

[Tim Horton]

Comment on attachment 237607 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=237607&action=review

> Source/WebCore/rendering/RenderLayer.cpp:6401
> +    bool needsHorizontalScrollbar = renderer().hasOverflowClip() && ((hasHorizontalScrollbar() && overflowDefinesAutomaticScrollbar(overflowX)) || overflowRequiresScrollbar(overflowX));

we already have our RenderBox in a local, perhaps use that instead?

>> LayoutTests/platform/mac-wk2/tiled-drawing/scrolling/root-overflow-with-mousewheel.html:35
>> +    setTimeout(checkForScroll, 500);
> 
> Oh, I can get rid of this setTimeout completely, so that's good.

Please do! 500ms tests are unwelcome :)

[Beth Dakin]

Thanks Tim! http://trac.webkit.org/changeset/173275