Summary: | Slow content causes choppy scrolling | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Moore <davemoore> | ||||||||
Component: | UI Events | Assignee: | Dave Moore <davemoore> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andersca, ap, fishd, jamesr, mitz, simon.fraser, tonikitoo, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Dave Moore
2012-02-23 14:44:10 PST
Created attachment 128754 [details]
Patch
Comment on attachment 128754 [details]
Patch
This patch contains unnecessary changes (the Gesture stuff). I will upload a modified one shortly.
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. Created attachment 128812 [details]
Patch
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 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); ? Created attachment 129312 [details]
Patch
Comment on attachment 129312 [details]
Patch
Awesome! R=me
Comment on attachment 129312 [details] Patch Clearing flags on attachment: 129312 Committed r109151: <http://trac.webkit.org/changeset/109151> All reviewed patches have been landed. Closing bug. |