Bug 79403 - Slow content causes choppy scrolling
Summary: Slow content causes choppy scrolling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dave Moore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-23 14:44 PST by Dave Moore
Modified: 2012-04-23 11:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.50 KB, patch)
2012-02-24 10:12 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2012-02-24 14:48 PST, Dave Moore
no flags Details | Formatted Diff | Diff
Patch (5.44 KB, patch)
2012-02-28 12:57 PST, Dave Moore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Moore 2012-02-23 14:44:10 PST
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.
Comment 1 Dave Moore 2012-02-24 10:12:24 PST
Created attachment 128754 [details]
Patch
Comment 2 Dave Moore 2012-02-24 11:57:28 PST
Comment on attachment 128754 [details]
Patch

This patch contains unnecessary changes (the Gesture stuff). I will upload a modified one shortly.
Comment 3 James Robinson 2012-02-24 13:36:28 PST
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.
Comment 4 Dave Moore 2012-02-24 14:48:29 PST
Created attachment 128812 [details]
Patch
Comment 5 Alexey Proskuryakov 2012-02-24 15:24:49 PST
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.
Comment 6 James Robinson 2012-02-28 11:27:00 PST
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);

?
Comment 7 Dave Moore 2012-02-28 12:57:20 PST
Created attachment 129312 [details]
Patch
Comment 8 James Robinson 2012-02-28 13:18:44 PST
Comment on attachment 129312 [details]
Patch

Awesome! R=me
Comment 9 WebKit Review Bot 2012-02-28 14:26:40 PST
Comment on attachment 129312 [details]
Patch

Clearing flags on attachment: 129312

Committed r109151: <http://trac.webkit.org/changeset/109151>
Comment 10 WebKit Review Bot 2012-02-28 14:26:45 PST
All reviewed patches have been landed.  Closing bug.