Consolidate WebInputEvent coalescing logic

Consolidate WebInputEvent coalescing logic

Code for coalescing the various WebInputEvent types is littered throughout the
code.  Move all such logic into WebInputEventTraits, making possible standalone
test for event coalescing, and simplifying code that relies on coalescing.

BUG=303238
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228828

[jdduke (slow), 2013-10-10 21:18:43]

aelias@: PTAL.  This is simply moving all (event + event) coalescing logic to
web_input_event_traits.{h,cc}.  There's some more complicated multi-event
coalescing in GestureEventFilter for pinch/scroll, but I think it's fine to
leave as is for the time being.

This greatly simplifies code that relies on coalescing, making it easier to
reason about coalescing response rather than just doing the coalescing.  I went
back and forth on making "TryCoalesce" into "CanCoalesce + Coalesce", but I'm
not really married to either approach.

[jdduke (slow), 2013-10-10 21:21:26]

https://codereview.chromium.org/26923002/diff/1/content/common/input/web_inpu...
File content/common/input/web_input_event_traits.h (right):

https://codereview.chromium.org/26923002/diff/1/content/common/input/web_inpu...
content/common/input/web_input_event_traits.h:20: static bool TryCoalesce(const
WebKit::WebInputEvent& event_to_coalesce,
Any guidance on arg ordering here? I prefer "TryCoalesceWith(event,
const_event)", but I vaguely recall a Chromium preference for trailing pointer
args. Maybe just rename "TryCoalesceInto"?

[aelias_OOO_until_Jul13, 2013-10-14 03:38:57]

I'm thinking it would be better to split TryCoalesce into two methods,
IsCoalesceableWith() and Coalesce().  The latter could run the former within a
DCHECK.  There would be two benefits: the callsites would be easier to
understand, and if somebody wants to just check whether two events are
coalesceable in the future that will be available.

https://codereview.chromium.org/26923002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/gesture_event_filter.cc (right):

https://codereview.chromium.org/26923002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:373: if
(last_event->TryCoalesceWith(gesture_event)) {
nit: no {}

https://codereview.chromium.org/26923002/diff/1/content/common/input/web_inpu...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/26923002/diff/1/content/common/input/web_inpu...
content/common/input/web_input_event_traits.cc:24: if (unaccelerated_delta ==
0.f || accelerated_delta == 0.f)
I know you're just moving this, but this kind of divide-by-zero avoidance is
usually an error.  It would be better to DCHECK it's nonzero.

[jdduke (slow), 2013-10-14 17:03:17]

OK, updated accordingly with a query + command.  PTAL.

https://codereview.chromium.org/26923002/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/input/gesture_event_filter.cc (right):

https://codereview.chromium.org/26923002/diff/1/content/browser/renderer_host...
content/browser/renderer_host/input/gesture_event_filter.cc:373: if
(last_event->TryCoalesceWith(gesture_event)) {
On 2013/10/14 03:38:57, aelias wrote:
> nit: no {}

Done.

https://codereview.chromium.org/26923002/diff/1/content/common/input/web_inpu...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/26923002/diff/1/content/common/input/web_inpu...
content/common/input/web_input_event_traits.cc:24: if (unaccelerated_delta ==
0.f || accelerated_delta == 0.f)
On 2013/10/14 03:38:57, aelias wrote:
> I know you're just moving this, but this kind of divide-by-zero avoidance is
> usually an error.  It would be better to DCHECK it's nonzero.

Hmm, normally I'd agree, but it looks like they're using the wheel deltaX/Y as
arguments here, which may very well be zero.   In that case, the formula really
doesn't make sense, but... I could add guards before the function? Or maybe I'll
just move the function closer to where it's used.

[aelias_OOO_until_Jul13, 2013-10-14 19:05:58]

lgtm modulo comment below.

https://codereview.chromium.org/26923002/diff/11001/content/common/input/web_...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/26923002/diff/11001/content/common/input/web_...
content/common/input/web_input_event_traits.cc:37: int x = event->movementX;
Please add DCHECK(CanCoalesce(event_to_coalsece, *event)); here and in the
others.

[jdduke (slow), 2013-10-14 19:10:12]

https://codereview.chromium.org/26923002/diff/11001/content/common/input/web_...
File content/common/input/web_input_event_traits.cc (right):

https://codereview.chromium.org/26923002/diff/11001/content/common/input/web_...
content/common/input/web_input_event_traits.cc:37: int x = event->movementX;
On 2013/10/14 19:05:58, aelias wrote:
> Please add DCHECK(CanCoalesce(event_to_coalsece, *event)); here and in the
> others.

OK.  I had the DCHECK in WebInputEventTraits::Coalesce, but maybe having it
closer to the end usage is better.

[jdduke (slow), 2013-10-14 20:04:58]

joi@: Review for content/port/browser/? Thanks.

[Jói, 2013-10-15 19:12:21]

LGTM

[commit-bot: I haz the power, 2013-10-15 22:19:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdduke@chromium.org/26923002/39001

[commit-bot: I haz the power, 2013-10-16 01:34:21]

Change committed as 228828