Mac: Coalesce scroll events.

Mac: Coalesce scroll events.

Based on work by trungl, codereview issue 502056. He's busy, but I think this is an important patch, so I'll try to get it through review for him.

BUG=28954
TEST=On a MacBook (Pro) -- the slower the better -- go to <http://www.starcraft2.com/faq.xml>; and make sure that scrolling using a two-fingered swipe on the trackpad is much faster than before. Also make sure that using the mouse wheel to scroll remains okay with various devices on all platforms.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=36422

[Nico, 2010-01-14 06:23:10]

This makes scrolling much faster for me on certain pages, so I'd love to get
this in soon.

Darin: Do you know if the renderer does repaint coalescing? I.e. if events come
in at 100 Hz,  is the renderer smart enough to redraw at only 30 Hz or so?

[Mark Mentovai, 2010-01-14 17:27:28]

Needs work.

http://codereview.chromium.org/536059/diff/1001/1002
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/536059/diff/1001/1002#newcode386
chrome/browser/renderer_host/render_widget_host.cc:386:
coalesced_mouse_wheel_->deltaX += wheel_event.deltaX;
I seem to recall that some web apps wouldn't deal properly with mouse events
that could scroll both x and y simultaneously.

Certainly, with current Apple hardware, it's possible to scroll x and y
simultaneously.  Coalescing means that we'll be exposed to this behavior in many
more cases, even when the hardware doesn't scroll both axes at the same time.

Have you looked into this and what sorts of problems it might aggravate?

http://codereview.chromium.org/536059/diff/1001/1002#newcode390
chrome/browser/renderer_host/render_widget_host.cc:390:
coalesced_mouse_wheel_->modifiers |= wheel_event.modifiers;
This is wrong.

(The error seems obvious to me, but in case it's not to you, ask me and I'll
explain more.)

http://codereview.chromium.org/536059/diff/1001/1002#newcode393
chrome/browser/renderer_host/render_widget_host.cc:393:
coalesced_mouse_wheel_->timeStampSeconds = wheel_event.timeStampSeconds;
I think std::max would be clearer.

http://codereview.chromium.org/536059/diff/1001/1002#newcode395
chrome/browser/renderer_host/render_widget_host.cc:395:
coalesced_mouse_wheel_->scrollByPage = wheel_event.scrollByPage;
This is wrong, similar to line 390.

[Mark Mentovai, 2010-01-14 17:29:18]

I suggest maintaining a queue of coalesced wheel events.  When you're looking to
push (or coalesce) a new wheel event e, coalesce it with q.end only if
q.end.flags are compatible with e.flags.  Otherwise, push new wheel event e
instead of coalescing.

[Nico, 2010-01-14 18:26:28]

I will look into the queue idea.o do some reasing

http://codereview.chromium.org/536059/diff/1001/1002
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/536059/diff/1001/1002#newcode386
chrome/browser/renderer_host/render_widget_host.cc:386:
coalesced_mouse_wheel_->deltaX += wheel_event.deltaX;
On 2010/01/14 17:27:28, Mark Mentovai wrote:
> I seem to recall that some web apps wouldn't deal properly with mouse events
> that could scroll both x and y simultaneously.
> 
> Certainly, with current Apple hardware, it's possible to scroll x and y
> simultaneously.  Coalescing means that we'll be exposed to this behavior in
many
> more cases, even when the hardware doesn't scroll both axes at the same time.
> 
> Have you looked into this and what sorts of problems it might aggravate?

I haven't. Note that this would only happen if a mouse wheel event with both
deltaX and deltaY gets coalesced, which on non-mac hardware only happens if you
use the scrollwheel and then very quickly shift-scrollwheel.

I will read up on how wheel events work in javascript and then comment on this
again.

If I do this queue thing you suggest, I could not merge events if they disagree
in deltaX/deltaY, but that would break the mac use case. Let's see.

http://codereview.chromium.org/536059/diff/1001/1002#newcode390
chrome/browser/renderer_host/render_widget_host.cc:390:
coalesced_mouse_wheel_->modifiers |= wheel_event.modifiers;
On 2010/01/14 17:27:28, Mark Mentovai wrote:
> This is wrong.
> 
> (The error seems obvious to me, but in case it's not to you, ask me and I'll
> explain more.)

You're not on IRC :-)

http://codereview.chromium.org/536059/diff/1001/1002#newcode393
chrome/browser/renderer_host/render_widget_host.cc:393:
coalesced_mouse_wheel_->timeStampSeconds = wheel_event.timeStampSeconds;
On 2010/01/14 17:27:28, Mark Mentovai wrote:
> I think std::max would be clearer.

K, will do.

[darin (slow to review), 2010-01-15 18:23:50]

Seems completely reasonable...

http://codereview.chromium.org/536059/diff/2004/2005
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/536059/diff/2004/2005#newcode394
chrome/browser/renderer_host/render_widget_host.cc:394:
coalesced_mouse_wheel_events_.back().timeStampSeconds = std::max(
it seems like the timestamp of the newly received input event should always be
greater than or equal to the timestamp of the existing event.  perhaps you
should just assign and DCHECK that it was >= ?

http://codereview.chromium.org/536059/diff/2004/2005#newcode402
chrome/browser/renderer_host/render_widget_host.cc:402:
HISTOGRAM_COUNTS_100("Renderer.WheelQueueSize",
please use the "MPArch." prefix here.

it is too bad that Renderer.KeyboardQueueSize is not also using the MPArch
prefix.

http://codereview.chromium.org/536059/diff/2004/2005#newcode779
chrome/browser/renderer_host/render_widget_host.cc:779:
WebKit::WebMouseWheelEvent next_wheel_event =
please drop the WebKit:: prefix here.

http://codereview.chromium.org/536059/diff/2004/2006
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/536059/diff/2004/2006#newcode527
chrome/browser/renderer_host/render_widget_host.h:527: WheelEventQueue
coalesced_mouse_wheel_events_;
it seems like in the typical case the modifier won't change.  how about just
sending the queued mouse wheel event when a modifier changes?

[Mark Mentovai, 2010-01-15 18:56:19]

darin wrote:
> it seems like in the typical case the modifier won't change.  how about
> just sending the queued mouse wheel event when a modifier changes?

Yeah, sounds like that'd work.

Mark

[Nico, 2010-01-15 19:01:12]

Added a test.

http://codereview.chromium.org/536059/diff/1001/1002
File chrome/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/536059/diff/1001/1002#newcode386
chrome/browser/renderer_host/render_widget_host.cc:386:
coalesced_mouse_wheel_->deltaX += wheel_event.deltaX;
On 2010/01/14 18:26:28, Nico wrote:
> On 2010/01/14 17:27:28, Mark Mentovai wrote:
> > I seem to recall that some web apps wouldn't deal properly with mouse events
> > that could scroll both x and y simultaneously.
> > 
> > Certainly, with current Apple hardware, it's possible to scroll x and y
> > simultaneously.  Coalescing means that we'll be exposed to this behavior in
> many
> > more cases, even when the hardware doesn't scroll both axes at the same
time.
> > 
> > Have you looked into this and what sorts of problems it might aggravate?
> 
> I haven't. Note that this would only happen if a mouse wheel event with both
> deltaX and deltaY gets coalesced, which on non-mac hardware only happens if
you
> use the scrollwheel and then very quickly shift-scrollwheel.
> 
> I will read up on how wheel events work in javascript and then comment on this
> again.

It looks like the "normal" DOMMouseWheel / mousewheel event only sends one value
(y) anyway. wheelDeltaX/Y seem to be a webkit addition, and safari sets both !=
0 at the same time. So this should be fine.

http://codereview.chromium.org/536059/diff/2004/2005#newcode394
chrome/browser/renderer_host/render_widget_host.cc:394: // |scrollByPage| is a
bool.
On 2010/01/15 18:23:50, darin wrote:
> it seems like the timestamp of the newly received input event should always be
> greater than or equal to the timestamp of the existing event.  perhaps you
> should just assign and DCHECK that it was >= ?

Done.

http://codereview.chromium.org/536059/diff/2004/2005#newcode402
chrome/browser/renderer_host/render_widget_host.cc:402: 
On 2010/01/15 18:23:50, darin wrote:
> please use the "MPArch." prefix here.
> 
> it is too bad that Renderer.KeyboardQueueSize is not also using the MPArch
> prefix.

Done.

http://codereview.chromium.org/536059/diff/2004/2005#newcode779
chrome/browser/renderer_host/render_widget_host.cc:779: } else if
(WebInputEvent::isKeyboardEventType(type)) {
On 2010/01/15 18:23:50, darin wrote:
> please drop the WebKit:: prefix here.

Done.

http://codereview.chromium.org/536059/diff/2004/2006
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/536059/diff/2004/2006#newcode527
chrome/browser/renderer_host/render_widget_host.h:527: WheelEventQueue
coalesced_mouse_wheel_events_;
On 2010/01/15 18:23:50, darin wrote:
> it seems like in the typical case the modifier won't change.  how about just
> sending the queued mouse wheel event when a modifier changes?

That probably works too. The queue behavior is slightly nicer to test and
effectively the same in practice (and it's also what I currently have :-P), so
I'd keep the queue for now if that's ok.

[darin (slow to review), 2010-01-15 19:23:44]

On Fri, Jan 15, 2010 at 11:01 AM, <thakis@chromium.org> wrote:

> Added a test.
>
>
> http://codereview.chromium.org/536059/diff/1001/1002
>
> File chrome/browser/renderer_host/render_widget_host.cc (right):
>
> http://codereview.chromium.org/536059/diff/1001/1002#newcode386
> chrome/browser/renderer_host/render_widget_host.cc:386:
> coalesced_mouse_wheel_->deltaX += wheel_event.deltaX;
> On 2010/01/14 18:26:28, Nico wrote:
>
>> On 2010/01/14 17:27:28, Mark Mentovai wrote:
>> > I seem to recall that some web apps wouldn't deal properly with
>>
> mouse events
>
>> > that could scroll both x and y simultaneously.
>> >
>> > Certainly, with current Apple hardware, it's possible to scroll x
>>
> and y
>
>> > simultaneously.  Coalescing means that we'll be exposed to this
>>
> behavior in
>
>> many
>> > more cases, even when the hardware doesn't scroll both axes at the
>>
> same time.
>
>> >
>> > Have you looked into this and what sorts of problems it might
>>
> aggravate?
>
>  I haven't. Note that this would only happen if a mouse wheel event
>>
> with both
>
>> deltaX and deltaY gets coalesced, which on non-mac hardware only
>>
> happens if you
>
>> use the scrollwheel and then very quickly shift-scrollwheel.
>>
>
>  I will read up on how wheel events work in javascript and then comment
>>
> on this
>
>> again.
>>
>
> It looks like the "normal" DOMMouseWheel / mousewheel event only sends
> one value (y) anyway. wheelDeltaX/Y seem to be a webkit addition, and
> safari sets both != 0 at the same time. So this should be fine.
>
>
> http://codereview.chromium.org/536059/diff/2004/2005#newcode394
> chrome/browser/renderer_host/render_widget_host.cc:394: //
> |scrollByPage| is a bool.
>
> On 2010/01/15 18:23:50, darin wrote:
>
>> it seems like the timestamp of the newly received input event should
>>
> always be
>
>> greater than or equal to the timestamp of the existing event.  perhaps
>>
> you
>
>> should just assign and DCHECK that it was >= ?
>>
>
> Done.
>
>
> http://codereview.chromium.org/536059/diff/2004/2005#newcode402
> chrome/browser/renderer_host/render_widget_host.cc:402:
> On 2010/01/15 18:23:50, darin wrote:
>
>> please use the "MPArch." prefix here.
>>
>
>  it is too bad that Renderer.KeyboardQueueSize is not also using the
>>
> MPArch
>
>> prefix.
>>
>
> Done.
>
>
> http://codereview.chromium.org/536059/diff/2004/2005#newcode779
> chrome/browser/renderer_host/render_widget_host.cc:779: } else if
> (WebInputEvent::isKeyboardEventType(type)) {
>
> On 2010/01/15 18:23:50, darin wrote:
>
>> please drop the WebKit:: prefix here.
>>
>
> Done.
>
>
> http://codereview.chromium.org/536059/diff/2004/2006
> File chrome/browser/renderer_host/render_widget_host.h (right):
>
> http://codereview.chromium.org/536059/diff/2004/2006#newcode527
> chrome/browser/renderer_host/render_widget_host.h:527: WheelEventQueue
> coalesced_mouse_wheel_events_;
> On 2010/01/15 18:23:50, darin wrote:
>
>> it seems like in the typical case the modifier won't change.  how
>>
> about just
>
>> sending the queued mouse wheel event when a modifier changes?
>>
>
> That probably works too. The queue behavior is slightly nicer to test
> and effectively the same in practice (and it's also what I currently
> have :-P), so I'd keep the queue for now if that's ok.



OK


>
>
> http://codereview.chromium.org/536059
>

[Mark Mentovai, 2010-01-15 21:46:10]

lgtm.