[webkit-reviews] review denied: [Bug 79403] Slow content causes choppy scrolling : [Attachment 128812] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Dave Moore
<davemoore at google.com>'s request for review:
Bug 79403: Slow content causes choppy scrolling
https://bugs.webkit.org/show_bug.cgi?id=79403

Attachment 128812: Patch
https://bugs.webkit.org/attachment.cgi?id=128812&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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);

?