[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into 1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

Note that we have to go through the matrix when there are 2 adjacent pinch events
with different pinch point as they would coalesce to 1 scroll event and 1 pinch event.

BUG=661601
Committed: https://crrev.com/058c4c7ec00da18c90214ecaa956a84e229548ad
Cr-Commit-Position: refs/heads/master@{#438534}

[chongz, 2016-12-05 21:41:36]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 21:42:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 21:47:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 21:47:25]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[chongz, 2016-12-06 15:32:12]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 15:32:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 15:35:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 15:35:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2016-12-06 15:44:29]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 15:44:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-12-06 15:45:58]

Patchset #1 (id:1) has been deleted

[chongz, 2016-12-06 15:46:05]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-12-06 15:48:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 15:48:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chongz, 2016-12-06 15:50:14]

Description was changed from

==========
Support coalescing gesture scroll&pinch in one sequence for compositor event
queue

BUG=661601
==========

to

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

BUG=661601
==========

[chongz, 2016-12-06 15:51:53]

Description was changed from

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

BUG=661601
==========

to

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

BUG=661601
==========

[chongz, 2016-12-06 15:59:41]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 16:00:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-12-06 16:30:32]

Patchset #1 (id:40001) has been deleted

[chongz, 2016-12-06 16:53:58]

chongz@chromium.org changed reviewers:
+ dtapuska@chromium.org

[chongz, 2016-12-06 16:53:59]

dtapuska@ PTAL, thanks!

[commit-bot: I haz the power, 2016-12-06 16:58:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 16:58:25]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chongz, 2016-12-06 18:32:59]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 18:33:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-12-06 18:35:22]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-12-06 19:34:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 19:34:07]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-12-08 14:53:16]

chongz@chromium.org changed reviewers:
+ tdresser@chromium.org

[chongz, 2016-12-08 14:53:16]

tdresser@ PTAL, thanks!

[dtapuska, 2016-12-08 15:18:37]

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/composi...
File ui/events/blink/compositor_thread_event_queue.cc (right):

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/composi...
ui/events/blink/compositor_thread_event_queue.cc:42:
std::pair<blink::WebGestureEvent, blink::WebGestureEvent> coalesced_events =
Isn't this complicated logic if second_last_event is null?

Have we thought about always ensuring GSU's always come before GPU's and then
could we just do this coalescing in place?

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/event_w...
File ui/events/blink/event_with_callback.h (right):

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/event_w...
ui/events/blink/event_with_callback.h:50: const blink::WebGestureEvent&
ToWebGestureEvent() const;
This method looks a little weird to have on something that takes a general
WebInputEvent

[chongz, 2016-12-09 20:46:25]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 20:47:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-12-09 20:52:25]

dtapuska@ I've updated as per our discussion, PTAL again, thanks!

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/composi...
File ui/events/blink/compositor_thread_event_queue.cc (right):

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/composi...
ui/events/blink/compositor_thread_event_queue.cc:42:
std::pair<blink::WebGestureEvent, blink::WebGestureEvent> coalesced_events =
On 2016/12/08 15:18:37, dtapuska wrote:
> Isn't this complicated logic if second_last_event is null?
> 
> Have we thought about always ensuring GSU's always come before GPU's and then
> could we just do this coalescing in place?

As discussed we have to go through the matrix as 2 pinch events with different
pinch point could also coalesce to 1 scroll event and 1 pinch event.

I've moved the logic of calculating other attributes and call pop_back() earlier
as per our discussion.

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/event_w...
File ui/events/blink/event_with_callback.h (right):

https://codereview.chromium.org/2552853002/diff/80001/ui/events/blink/event_w...
ui/events/blink/event_with_callback.h:50: const blink::WebGestureEvent&
ToWebGestureEvent() const;
On 2016/12/08 15:18:37, dtapuska wrote:
> This method looks a little weird to have on something that takes a general
> WebInputEvent

Moved to "blink_event_util.h".

[chongz, 2016-12-09 20:55:16]

Description was changed from

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

BUG=661601
==========

to

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

Note that we have to go through the matrix when there are 2 adjacent pinch
events
with different pinch point as they would coalesce to 1 scroll event and 1 pinch
event.

BUG=661601
==========

[commit-bot: I haz the power, 2016-12-09 22:56:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 22:56:52]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2016-12-13 15:57:23]

https://codereview.chromium.org/2552853002/diff/100001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/2552853002/diff/100001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:316:
ui::WebInputEventTraits::Clone(coalesced_gesture_events_.back().event);
Does last_gesture_event really need to be heap allocated? I think you can just
put the object on the stack.

https://codereview.chromium.org/2552853002/diff/100001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.h (right):

https://codereview.chromium.org/2552853002/diff/100001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.h:97: const blink::WebGestureEvent*
ToWebGestureEvent(const blink::WebInputEvent*);
Can't these be inline functions?

[chongz, 2016-12-13 16:24:54]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 16:25:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-12-13 16:30:44]

dtapuska@ I've updated as per your comments, PTAL again, thanks!

https://codereview.chromium.org/2552853002/diff/100001/content/browser/render...
File content/browser/renderer_host/input/gesture_event_queue.cc (right):

https://codereview.chromium.org/2552853002/diff/100001/content/browser/render...
content/browser/renderer_host/input/gesture_event_queue.cc:316:
ui::WebInputEventTraits::Clone(coalesced_gesture_events_.back().event);
On 2016/12/13 15:57:22, dtapuska wrote:
> Does last_gesture_event really need to be heap allocated? I think you can just
> put the object on the stack.

Done.

https://codereview.chromium.org/2552853002/diff/100001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.h (right):

https://codereview.chromium.org/2552853002/diff/100001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.h:97: const blink::WebGestureEvent*
ToWebGestureEvent(const blink::WebInputEvent*);
On 2016/12/13 15:57:23, dtapuska wrote:
> Can't these be inline functions?

Done.

[dtapuska, 2016-12-13 16:33:43]

On 2016/12/13 16:30:44, chongz wrote:
> dtapuska@ I've updated as per your comments, PTAL again, thanks!
> 
>
https://codereview.chromium.org/2552853002/diff/100001/content/browser/render...
> File content/browser/renderer_host/input/gesture_event_queue.cc (right):
> 
>
https://codereview.chromium.org/2552853002/diff/100001/content/browser/render...
> content/browser/renderer_host/input/gesture_event_queue.cc:316:
> ui::WebInputEventTraits::Clone(coalesced_gesture_events_.back().event);
> On 2016/12/13 15:57:22, dtapuska wrote:
> > Does last_gesture_event really need to be heap allocated? I think you can
just
> > put the object on the stack.
> 
> Done.
> 
>
https://codereview.chromium.org/2552853002/diff/100001/ui/events/blink/blink_...
> File ui/events/blink/blink_event_util.h (right):
> 
>
https://codereview.chromium.org/2552853002/diff/100001/ui/events/blink/blink_...
> ui/events/blink/blink_event_util.h:97: const blink::WebGestureEvent*
> ToWebGestureEvent(const blink::WebInputEvent*);
> On 2016/12/13 15:57:23, dtapuska wrote:
> > Can't these be inline functions?
> 
> Done.

lgtm but wait for tdresser@'s review too

[chongz, 2016-12-13 16:35:06]

tdresser@ PTAL, thanks!

[tdresser, 2016-12-13 17:18:19]

LGTM

https://codereview.chromium.org/2552853002/diff/120001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.h (right):

https://codereview.chromium.org/2552853002/diff/120001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.h:96: inline const blink::WebGestureEvent*
ToWebGestureEvent(
The asymmetry of these methods is a bit tricky. Could we make it so neither or
both of them take a pointer?

[commit-bot: I haz the power, 2016-12-13 17:34:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 17:34:18]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-12-13 18:43:50]

The CQ bit was checked by chongz@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 18:44:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chongz, 2016-12-13 18:55:45]

https://codereview.chromium.org/2552853002/diff/120001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.h (right):

https://codereview.chromium.org/2552853002/diff/120001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.h:96: inline const blink::WebGestureEvent*
ToWebGestureEvent(
On 2016/12/13 17:18:19, tdresser wrote:
> The asymmetry of these methods is a bit tricky. Could we make it so neither or
> both of them take a pointer?

Removed the pointer version. 
However I'm not entirely sure if that's what you mean because both methods are
converting |WebInputEvent| to |WebGestureEvent|.

[tdresser, 2016-12-13 19:04:46]

Sorry, my comment was nonsensical, but what you did made sense.

Still LGTM.

[commit-bot: I haz the power, 2016-12-13 20:21:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 20:21:15]

Dry run: This issue passed the CQ dry run.

[chongz, 2016-12-13 20:25:22]

The CQ bit was checked by chongz@chromium.org

[chongz, 2016-12-13 20:25:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2552853002/#ps140001
(title: "tdresser's review, remov the pointer version of ToWebGestureEvent()")

[commit-bot: I haz the power, 2016-12-13 20:26:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 20:34:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 20:34:01]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chongz, 2016-12-13 20:37:19]

chongz@chromium.org changed reviewers:
+ sadrul@chromium.org

[chongz, 2016-12-13 20:37:20]

sadrul@ PTAL at "ui/events/blink/DEPS", thanks!

[sadrul, 2016-12-14 01:00:21]

lgtm

https://codereview.chromium.org/2552853002/diff/140001/ui/events/blink/BUILD.gn
File ui/events/blink/BUILD.gn (right):

https://codereview.chromium.org/2552853002/diff/140001/ui/events/blink/BUILD....
ui/events/blink/BUILD.gn:40: "//ui/gfx:gfx",
just ui/gfx

https://codereview.chromium.org/2552853002/diff/140001/ui/events/blink/DEPS
File ui/events/blink/DEPS (right):

https://codereview.chromium.org/2552853002/diff/140001/ui/events/blink/DEPS#n...
ui/events/blink/DEPS:18: "+ui/gfx:gfx",
Just ui/gfx (is this even working?)

[chongz, 2016-12-14 15:55:16]

The CQ bit was checked by chongz@chromium.org

[chongz, 2016-12-14 15:55:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtapuska@chromium.org, sadrul@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/2552853002/#ps160001
(title: "sadrul's comments, fix dependency")

[commit-bot: I haz the power, 2016-12-14 15:55:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 16:58:31]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481730916479770,
"parent_rev": "f0ade73900d081ce4ce150825a9858d8d971a008", "commit_rev":
"9c4b765b73252f5b04bf2e776d280ef17040c549"}

[commit-bot: I haz the power, 2016-12-14 16:58:57]

Description was changed from

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

Note that we have to go through the matrix when there are 2 adjacent pinch
events
with different pinch point as they would coalesce to 1 scroll event and 1 pinch
event.

BUG=661601
==========

to

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

Note that we have to go through the matrix when there are 2 adjacent pinch
events
with different pinch point as they would coalesce to 1 scroll event and 1 pinch
event.

BUG=661601

Review-Url: https://codereview.chromium.org/2552853002
==========

[commit-bot: I haz the power, 2016-12-14 16:58:59]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-12-14 17:03:10]

Description was changed from

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

Note that we have to go through the matrix when there are 2 adjacent pinch
events
with different pinch point as they would coalesce to 1 scroll event and 1 pinch
event.

BUG=661601

Review-Url: https://codereview.chromium.org/2552853002
==========

to

==========
[Compositor event queue] Coalesce gesture scroll&pinch of the same sequence

It is possible to have |GestureScrollUpdate| and |GesturePinchUpdate| in the
same
|GestureScrollBegin/End| sequence. This CL coalesces multiple GSUs and GPUs into
1
GSU and 1 GPU using matrix transformation.

e.g.
|GSB| -> |GSU| -> |GSU| -> |GPU| -> |GSU| -> |GSE|
becomes
|GSB| -> |GSU|* -> |GPU|* -> |GSE|

* means coalesced events.

Note that we have to go through the matrix when there are 2 adjacent pinch
events
with different pinch point as they would coalesce to 1 scroll event and 1 pinch
event.

BUG=661601

Committed: https://crrev.com/058c4c7ec00da18c90214ecaa956a84e229548ad
Cr-Commit-Position: refs/heads/master@{#438534}
==========

[commit-bot: I haz the power, 2016-12-14 17:03:13]

Patchset 5 (id:??) landed as
https://crrev.com/058c4c7ec00da18c90214ecaa956a84e229548ad
Cr-Commit-Position: refs/heads/master@{#438534}