Bug 79403 – Slow content causes choppy scrolling

• Component: UI Events
• Status: RESOLVED FIXED

[Dave Moore]

If sites take too long to respond to mousemove or mouseover it interrupts the scroll. For example techcrunch.com can take a half second to update its ui as the mouse passes over articles. When scrolling with a wheel or pad, leaving the mouse pointer in the middle of the page, this causes the scroll to be extremely choppy.

[Dave Moore]

Created attachment 128754 [details]
Patch

[Dave Moore]

Comment on attachment 128754 [details]
Patch

This patch contains unnecessary changes (the Gesture stuff). I will upload a modified one shortly.

[James Robinson]

Style note for your information: <wtf/CurrentTime.h> puts currentTime() and friends into the global namespace with a using declaration and the convention in the rest of WebKit is to simply use the name without the WTF:: qualifier.  I also think this would be a good place to use monotonicallyIncreasingTime() instead of currentTime(), it's in the same header and has the same units (seconds) but can potentially have better precision and won't explode on you if NTP happens to adjust the system clock at an inopportune time.

[Dave Moore]

Created attachment 128812 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 128812 [details]
Patch

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

I don't have much insight about the idea of this patch, but I a style nit.

> Source/WebCore/page/EventHandler.cpp:1660
> +    GetMaxDuration getMaxDuration(&m_maxMouseMovedDuration);

Class and variable names cannot be verbs.

[James Robinson]

Comment on attachment 128812 [details]
Patch

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

I think the behavior here is really nice.  With a bit of style TLC (comments below) this looks good to land for me.

For context, the issue here is that some pages have very slow mousemove handlers.  Currently these fire while scrolling the page since we want to update the cursor/etc to reflect the content that moves 'under' the cursor.  The current behavior is that we set a short timer on the scroll that dispatches a synthetic mouse event 'soon' after the initial scroll.  If the user is trying to scroll continuously, however, the 'soon' ends up interfering with a later scroll.  This patch detects pages that have slow mousemove handlers and if so it pushes the timer out on every scroll so that it only fires when the user stops scrolling altogether.

Mozilla landed a similar fix recently where they suppress synthetic mousemoves during an animated scroll (https://bugzilla.mozilla.org/show_bug.cgi?id=675015).

> Source/WebCore/page/EventHandler.cpp:150
> +class GetMaxDuration {

As Alexey pointed out, we try to make class names be nouns in WebKit.  I think this might be called a MaximumDurationTracker or something of that nature

> Source/WebCore/page/EventHandler.cpp:152
> +    GetMaxDuration(double *maxDuration) : m_maxDuration(maxDuration), m_start(monotonicallyIncreasingTime()) { }

1-arg c'tors should have the 'explicit' keyword. can you expand this initialization out to have one statement per line? WebKit formatting would look something like:

explicit ClassName(double *maxDuration)
    : m_maxDuration(maxDuration)
    , m_start(monotonicallyIncreasingTime()
{
}

which looks a little weird but means fewer lines churn if people add/remove members in the future

> Source/WebCore/page/EventHandler.cpp:158
> +        double duration = monotonicallyIncreasingTime() - m_start;
> +        if (duration > *m_maxDuration)
> +            *m_maxDuration = duration;

how about just:

*m_maxDuration = max(*m_maxDuration, monotonicallyIncreasingTime() - m_start);

?

[Dave Moore]

Created attachment 129312 [details]
Patch

[James Robinson]

Comment on attachment 129312 [details]
Patch

Awesome! R=me

[WebKit Review Bot]

Comment on attachment 129312 [details]
Patch

Clearing flags on attachment: 129312

Committed r109151: <http://trac.webkit.org/changeset/109151>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.