Support Coalesced Touch in ppapi

Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=720529
Review-Url: https://codereview.chromium.org/2844823002
Cr-Commit-Position: refs/heads/master@{#471909}
Committed: https://chromium.googlesource.com/chromium/src/+/8e72a1beacc36f4f0e4d9cb3e4404cf8d896d931

[jkwang, 2017-04-28 17:13:27]

Description was changed from

==========
Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=
==========

to

==========
Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=
==========

[jkwang, 2017-04-28 17:13:28]

jkwang@google.com changed reviewers:
+ bbudge@chromium.org, nzolghadr@chromium.org

[Navid Zolghadr, 2017-05-01 15:53:32]

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

[Navid Zolghadr, 2017-05-01 15:53:33]

https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1128: // event.Event().
Is there any reason you are only doing this for touch events and not mouse
events?

https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: return
touch_event_manager_->HandleTouchEvent(event, coalesced_events,
This is unlikely that we keep this coalesced_events variable here as we are in
the process of removing the coalesced touch events altogether and just keep the
coalesced pointer events. In that case we will not have the grouping for the
coalesced touch events anymore.

[bbudge, 2017-05-01 18:49:06]

Will this work when the plugin is fullscreen? It seems like the old
HandleInputEvent routine is called in that case.

https://cs.chromium.org/chromium/src/content/renderer/render_widget_fullscree...

[jkwang, 2017-05-02 18:59:15]

https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1128: // event.Event().
On 2017/05/01 15:53:33, Navid Zolghadr wrote:
> Is there any reason you are only doing this for touch events and not mouse
> events?

There is no reason to not do it for mouse events. We can add the support for
mouse event later if we care(Is there a use case?). Currently, touch events
should be good enough for stylus support in ink.

https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):

https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: return
touch_event_manager_->HandleTouchEvent(event, coalesced_events,
On 2017/05/01 15:53:33, Navid Zolghadr wrote:
> This is unlikely that we keep this coalesced_events variable here as we are in
> the process of removing the coalesced touch events altogether and just keep
the
> coalesced pointer events. In that case we will not have the grouping for the
> coalesced touch events anymore.

Is there a time line of removing coalesced touch events?
If coalesced touch event is not available any more. We can build the touch
events from coalesced pointer events. The goal is to report the trace of all
touch points, so we can see touch event as a snapshot of "multiple pointer event
streams". 
I am not sure what I can do now to make things easier(and correct). But if we
can make the change when we remove the coalesced touch events, I think it's OK
to leave it like this.

[Navid Zolghadr, 2017-05-02 19:17:40]

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1122: bool result =
HandleInputEvent(event.Event(), cursor_info);
One thing here. Note that the event itself is the accumulation of all the
coalesced events. So you either don't care about the coalesced events and only
handle this event or should only handle the coalesced events. When you tried
this didn't this make any redundant points/lines in the drawings for example?

[dtapuska, 2017-05-02 19:18:27]

On 2017/05/02 18:59:15, jkwang wrote:
>
https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
> 
>
https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
> content/renderer/pepper/pepper_plugin_instance_impl.cc:1128: // event.Event().
> On 2017/05/01 15:53:33, Navid Zolghadr wrote:
> > Is there any reason you are only doing this for touch events and not mouse
> > events?
> 
> There is no reason to not do it for mouse events. We can add the support for
> mouse event later if we care(Is there a use case?). Currently, touch events
> should be good enough for stylus support in ink.
> 
>
https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> 
>
https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: return
> touch_event_manager_->HandleTouchEvent(event, coalesced_events,
> On 2017/05/01 15:53:33, Navid Zolghadr wrote:
> > This is unlikely that we keep this coalesced_events variable here as we are
in
> > the process of removing the coalesced touch events altogether and just keep
> the
> > coalesced pointer events. In that case we will not have the grouping for the
> > coalesced touch events anymore.
> 
> Is there a time line of removing coalesced touch events?
> If coalesced touch event is not available any more. We can build the touch
> events from coalesced pointer events. The goal is to report the trace of all
> touch points, so we can see touch event as a snapshot of "multiple pointer
event
> streams". 
> I am not sure what I can do now to make things easier(and correct). But if we
> can make the change when we remove the coalesced touch events, I think it's OK
> to leave it like this.

Are you able to build these coalesced points from the coalesced pointer events
instead then? If you are claiming that you'd need to do the work eventually
perhaps it is better to just do the work now and then we aren't blocked around
this? Otherwise you are adding something to the platform that we then have to
rewrite in order to continue with our plans.

[jkwang, 2017-05-02 20:27:44]

On 2017/05/02 19:18:27, dtapuska wrote:
> On 2017/05/02 18:59:15, jkwang wrote:
> >
>
https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
> > File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2844823002/diff/1/content/renderer/pepper/pep...
> > content/renderer/pepper/pepper_plugin_instance_impl.cc:1128: //
event.Event().
> > On 2017/05/01 15:53:33, Navid Zolghadr wrote:
> > > Is there any reason you are only doing this for touch events and not mouse
> > > events?
> > 
> > There is no reason to not do it for mouse events. We can add the support for
> > mouse event later if we care(Is there a use case?). Currently, touch events
> > should be good enough for stylus support in ink.
> > 
> >
>
https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2844823002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/input/PointerEventManager.cpp:321: return
> > touch_event_manager_->HandleTouchEvent(event, coalesced_events,
> > On 2017/05/01 15:53:33, Navid Zolghadr wrote:
> > > This is unlikely that we keep this coalesced_events variable here as we
are
> in
> > > the process of removing the coalesced touch events altogether and just
keep
> > the
> > > coalesced pointer events. In that case we will not have the grouping for
the
> > > coalesced touch events anymore.
> > 
> > Is there a time line of removing coalesced touch events?
> > If coalesced touch event is not available any more. We can build the touch
> > events from coalesced pointer events. The goal is to report the trace of all
> > touch points, so we can see touch event as a snapshot of "multiple pointer
> event
> > streams". 
> > I am not sure what I can do now to make things easier(and correct). But if
we
> > can make the change when we remove the coalesced touch events, I think it's
OK
> > to leave it like this.
> 
> Are you able to build these coalesced points from the coalesced pointer events
> instead then? If you are claiming that you'd need to do the work eventually
> perhaps it is better to just do the work now and then we aren't blocked around
> this? Otherwise you are adding something to the platform that we then have to
> rewrite in order to continue with our plans.

I am starting an email thread.

[jkwang, 2017-05-02 22:27:05]

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1122: bool result =
HandleInputEvent(event.Event(), cursor_info);
On 2017/05/02 19:17:40, Navid Zolghadr wrote:
> One thing here. Note that the event itself is the accumulation of all the
> coalesced events. So you either don't care about the coalesced events and only
> handle this event or should only handle the coalesced events. When you tried
> this didn't this make any redundant points/lines in the drawings for example?

No, it's not a problem here. movement_x and movement_y are coalesced. But ppapi
only use the position x and position y. It is not coalesced.

[Navid Zolghadr, 2017-05-02 23:57:57]

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1122: bool result =
HandleInputEvent(event.Event(), cursor_info);
On 2017/05/02 22:27:04, jkwang wrote:
> On 2017/05/02 19:17:40, Navid Zolghadr wrote:
> > One thing here. Note that the event itself is the accumulation of all the
> > coalesced events. So you either don't care about the coalesced events and
only
> > handle this event or should only handle the coalesced events. When you tried
> > this didn't this make any redundant points/lines in the drawings for
example?
> 
> No, it's not a problem here. movement_x and movement_y are coalesced. But
ppapi
> only use the position x and position y. It is not coalesced.

I still see this as not something that is desired. Now you will handle at least
two points with the exact same coordinates. Is there any other reason for just
not removing this line?

[jkwang, 2017-05-03 00:36:00]

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1122: bool result =
HandleInputEvent(event.Event(), cursor_info);
On 2017/05/02 23:57:56, Navid Zolghadr wrote:
> On 2017/05/02 22:27:04, jkwang wrote:
> > On 2017/05/02 19:17:40, Navid Zolghadr wrote:
> > > One thing here. Note that the event itself is the accumulation of all the
> > > coalesced events. So you either don't care about the coalesced events and
> only
> > > handle this event or should only handle the coalesced events. When you
tried
> > > this didn't this make any redundant points/lines in the drawings for
> example?
> > 
> > No, it's not a problem here. movement_x and movement_y are coalesced. But
> ppapi
> > only use the position x and position y. It is not coalesced.
> 
> I still see this as not something that is desired. Now you will handle at
least
> two points with the exact same coordinates. Is there any other reason for just
> not removing this line?

Note that the loop below is indexed from 1 instead of 0. Index 0 is the same as
event.Event(). Either way, I intend to return the result of handling the event
at index 0 (So I have to start the loop at 1).  Because it is more consistent
with previous implementation.

[Navid Zolghadr, 2017-05-03 02:30:03]

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1122: bool result =
HandleInputEvent(event.Event(), cursor_info);
On 2017/05/03 00:36:00, jkwang wrote:
> On 2017/05/02 23:57:56, Navid Zolghadr wrote:
> > On 2017/05/02 22:27:04, jkwang wrote:
> > > On 2017/05/02 19:17:40, Navid Zolghadr wrote:
> > > > One thing here. Note that the event itself is the accumulation of all
the
> > > > coalesced events. So you either don't care about the coalesced events
and
> > only
> > > > handle this event or should only handle the coalesced events. When you
> tried
> > > > this didn't this make any redundant points/lines in the drawings for
> > example?
> > > 
> > > No, it's not a problem here. movement_x and movement_y are coalesced. But
> > ppapi
> > > only use the position x and position y. It is not coalesced.
> > 
> > I still see this as not something that is desired. Now you will handle at
> least
> > two points with the exact same coordinates. Is there any other reason for
just
> > not removing this line?
> 
> Note that the loop below is indexed from 1 instead of 0. Index 0 is the same
as
> event.Event(). Either way, I intend to return the result of handling the event
> at index 0 (So I have to start the loop at 1).  Because it is more consistent
> with previous implementation.

Sorry. I didn't notice that 0. That also seems incorrect. Because if I recall
correctly the last index (at least its coordinates) should match the event at
the moment with the current implementation. But you shouldn't rely on that
either. We were planning to do some resampling for the event itself and align it
with the frame time stamp and keep all the actual events happened in the
coalesced events array. Either way handling/drawing both the event and the
coalesced events at the same time is incorrect.

[jkwang, 2017-05-03 18:45:39]

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/20001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1122: bool result =
HandleInputEvent(event.Event(), cursor_info);
On 2017/05/03 02:30:02, Navid Zolghadr wrote:
> On 2017/05/03 00:36:00, jkwang wrote:
> > On 2017/05/02 23:57:56, Navid Zolghadr wrote:
> > > On 2017/05/02 22:27:04, jkwang wrote:
> > > > On 2017/05/02 19:17:40, Navid Zolghadr wrote:
> > > > > One thing here. Note that the event itself is the accumulation of all
> the
> > > > > coalesced events. So you either don't care about the coalesced events
> and
> > > only
> > > > > handle this event or should only handle the coalesced events. When you
> > tried
> > > > > this didn't this make any redundant points/lines in the drawings for
> > > example?
> > > > 
> > > > No, it's not a problem here. movement_x and movement_y are coalesced.
But
> > > ppapi
> > > > only use the position x and position y. It is not coalesced.
> > > 
> > > I still see this as not something that is desired. Now you will handle at
> > least
> > > two points with the exact same coordinates. Is there any other reason for
> just
> > > not removing this line?
> > 
> > Note that the loop below is indexed from 1 instead of 0. Index 0 is the same
> as
> > event.Event(). Either way, I intend to return the result of handling the
event
> > at index 0 (So I have to start the loop at 1).  Because it is more
consistent
> > with previous implementation.
> 
> Sorry. I didn't notice that 0. That also seems incorrect. Because if I recall
> correctly the last index (at least its coordinates) should match the event at
> the moment with the current implementation. But you shouldn't rely on that
> either. We were planning to do some resampling for the event itself and align
it
> with the frame time stamp and keep all the actual events happened in the
> coalesced events array. Either way handling/drawing both the event and the
> coalesced events at the same time is incorrect.

Done.

[Navid Zolghadr, 2017-05-05 16:08:42]

lgtm

[dtapuska, 2017-05-05 16:21:10]

Where are the tests?

https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1125: bool result;
Probably need to initialize result?

https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1127: result =
HandleInputEvent(event.CoalescedEvent(i), cursor_info);
Should this be an |= ? Probably if any is handled then the event is handled?

[jkwang, 2017-05-05 20:21:20]

On 2017/05/05 16:21:10, dtapuska wrote:
> Where are the tests?
> 
>
https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
> 
>
https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
> content/renderer/pepper/pepper_plugin_instance_impl.cc:1125: bool result;
> Probably need to initialize result?
> 
>
https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
> content/renderer/pepper/pepper_plugin_instance_impl.cc:1127: result =
> HandleInputEvent(event.CoalescedEvent(i), cursor_info);
> Should this be an |= ? Probably if any is handled then the event is handled?

WebPluginContainerTest.* and TouchEventTest should cover the Webkit part. I also
added one touch scroll test with coalesced events. For the pepper part, it's
unfortunate that there is currently no unittest of
pepper_plugin_instance_impl.cc (Thus writing the first test seems quite
impossible). But I built my own client and tested it on kevin. It worked as
expected.

[jkwang, 2017-05-05 20:21:43]

https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1125: bool result;
On 2017/05/05 16:21:09, dtapuska wrote:
> Probably need to initialize result?

Done.

https://codereview.chromium.org/2844823002/diff/60001/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1127: result =
HandleInputEvent(event.CoalescedEvent(i), cursor_info);
On 2017/05/05 16:21:09, dtapuska wrote:
> Should this be an |= ? Probably if any is handled then the event is handled?

Done.

[jkwang, 2017-05-06 07:36:54]

The CQ bit was checked by jkwang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-06 07:37:40]

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, 2017-05-06 07:41:18]

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

[commit-bot: I haz the power, 2017-05-06 07:41:19]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jkwang, 2017-05-07 01:10:55]

The CQ bit was checked by jkwang@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-07 01:11:01]

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, 2017-05-07 03:08:03]

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

[commit-bot: I haz the power, 2017-05-07 03:08:03]

Dry run: This issue passed the CQ dry run.

[dtapuska, 2017-05-08 13:11:16]

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/TouchEvent.cpp:235:
native_event_.reset(new WebCoalescedInputEvent(event));
Can we add a DCHECK here that the WebCoalescedInputEvent type is a touch event?

See
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIn...

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp (right):

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp:665:
web_view->HandleInputEvent(coalesced_event);
You should likely store how many coalesced events were seen in the
EventTestPlugin override
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebP...)
and then have an EXPECT_EQ here...

[jkwang, 2017-05-08 17:26:53]

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/events/TouchEvent.cpp (right):

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/events/TouchEvent.cpp:235:
native_event_.reset(new WebCoalescedInputEvent(event));
On 2017/05/08 13:11:15, dtapuska wrote:
> Can we add a DCHECK here that the WebCoalescedInputEvent type is a touch
event?
> 
> See
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebIn...

Done.

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp (right):

https://codereview.chromium.org/2844823002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/tests/WebPluginContainerTest.cpp:665:
web_view->HandleInputEvent(coalesced_event);
On 2017/05/08 13:11:15, dtapuska wrote:
> You should likely store how many coalesced events were seen in the
> EventTestPlugin override
>
(https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebP...)
> and then have an EXPECT_EQ here...

Done.

[dtapuska, 2017-05-08 17:29:14]

lgtm

[jkwang, 2017-05-08 17:45:19]

The CQ bit was checked by jkwang@google.com

[jkwang, 2017-05-08 17:45:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
nzolghadr@chromium.org
Link to the patchset: https://codereview.chromium.org/2844823002/#ps120001
(title: "Support Coalesced Touch in ppapi")

[commit-bot: I haz the power, 2017-05-08 17:45:54]

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, 2017-05-08 17:58:24]

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

[commit-bot: I haz the power, 2017-05-08 17:58:25]

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...)

[jkwang, 2017-05-09 17:27:56]

jkwang@google.com changed reviewers:
+ jochen@chromium.org, pfeldman@chromium.org

[tommycli, 2017-05-09 18:06:11]

tommycli@chromium.org changed reviewers:
+ tommycli@chromium.org

[tommycli, 2017-05-09 18:06:12]

components/plugins LGTM with nit

https://codereview.chromium.org/2844823002/diff/120001/components/plugins/ren...
File components/plugins/renderer/webview_plugin.cc (right):

https://codereview.chromium.org/2844823002/diff/120001/components/plugins/ren...
components/plugins/renderer/webview_plugin.cc:31: using
blink::WebCoalescedInputEvent;
nit: Remove both blink::WebCoalescedInputEvent and blink::WebInputEvent from
this list. Not worth the using statements.

[piman, 2017-05-09 21:24:12]

piman@chromium.org changed reviewers:
+ piman@chromium.org

[piman, 2017-05-09 21:24:13]

lgtm

[Rick Byers, 2017-05-09 21:30:17]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2017-05-09 21:30:17]

lgtm

[Rick Byers, 2017-05-09 21:32:47]

BTW it looks like in addition to supporting coalesced events, this also fixes an
issue with the co-ordinate space when scrolled, right?  Please find or file a
bug for that and reference it in the commit description.  I vaguely remember
hearing something about touch events seeming broken in some plugin scenarios
(maybe just dsinclair talking about PDF issues), and I guess it might be
possible that "fixing" this bug will break some plugin which was working around
the bug, right?

[jkwang, 2017-05-09 21:36:06]

On 2017/05/09 21:32:47, Rick Byers wrote:
> BTW it looks like in addition to supporting coalesced events, this also fixes
an
> issue with the co-ordinate space when scrolled, right?  Please find or file a
> bug for that and reference it in the commit description.  I vaguely remember
> hearing something about touch events seeming broken in some plugin scenarios
> (maybe just dsinclair talking about PDF issues), and I guess it might be
> possible that "fixing" this bug will break some plugin which was working
around
> the bug, right?

I did not try to fix "co-ordinate space when scrolled" issue. For most parts of
the code, the cl just wrap WebTouchEvent into a WebCoalescedInputEvent and
unwrap it. Only the pepper api use the coalesced touch events.

[jkwang, 2017-05-09 21:41:26]

The CQ bit was checked by jkwang@google.com

[jkwang, 2017-05-09 21:41:27]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
piman@chromium.org, tommycli@chromium.org, nzolghadr@chromium.org,
dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2844823002/#ps140001
(title: "Support Coalesced Touch in ppapi")

[commit-bot: I haz the power, 2017-05-09 21:42:53]

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, 2017-05-09 23:40:51]

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

[commit-bot: I haz the power, 2017-05-09 23:40:52]

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

[jochen (gone - plz use gerrit), 2017-05-10 08:22:12]

please file a bug about what you're trying to achieve and reference it from the
CL description.

In general, I'd expect a somewhat lengthy justification for any change to PPAPI,
as I'm operating under the assumption that the API is frozen at this point.

[jkwang, 2017-05-10 17:40:19]

Description was changed from

==========
Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=
==========

to

==========
Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=720529
==========

[bbudge, 2017-05-10 17:52:03]

ppapi & content/renderer/pepper lgtm

[jkwang, 2017-05-10 18:51:04]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-10 18:52:22]

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, 2017-05-10 21:11:02]

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

[commit-bot: I haz the power, 2017-05-10 21:11:04]

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

[jkwang, 2017-05-10 22:47:12]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-10 23:56:33]

CQ is trying da patch.

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

[jkwang, 2017-05-11 00:00:38]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-11 00:03:46]

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, 2017-05-11 02:57:17]

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

[commit-bot: I haz the power, 2017-05-11 02:57:19]

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

[jkwang, 2017-05-11 16:48:41]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-11 16:49:11]

CQ is trying da patch.

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

[jochen (gone - plz use gerrit), 2017-05-11 16:49:53]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-05-11 16:50:57]

not lgtm... still waiting for an answer to my question

[jkwang, 2017-05-11 16:52:27]

On 2017/05/11 16:50:57, jochen wrote:
> not lgtm... still waiting for an answer to my question

Oh, sorry. What's your LDAP? Let's talk offline

[jochen (gone - plz use gerrit), 2017-05-15 05:30:13]

thanks for sending the design doc. lgtm.

[jkwang, 2017-05-15 05:43:56]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-15 05:44:16]

CQ is trying da patch.

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

[jkwang, 2017-05-15 05:44:16]

The CQ bit was checked by jkwang@google.com

[jkwang, 2017-05-15 05:44:17]

The CQ bit was unchecked by jkwang@google.com

[jkwang, 2017-05-15 15:24:24]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-15 15:24:44]

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, 2017-05-15 15:29:57]

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

[commit-bot: I haz the power, 2017-05-15 15:30:00]

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...)

[jkwang, 2017-05-15 16:37:05]

The CQ bit was checked by jkwang@google.com

[jkwang, 2017-05-15 16:37:06]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org,
jochen@chromium.org, rbyers@chromium.org, piman@chromium.org,
tommycli@chromium.org, nzolghadr@chromium.org, dtapuska@chromium.org
Link to the patchset: https://codereview.chromium.org/2844823002/#ps160001
(title: "Support Coalesced Touch in ppapi")

[commit-bot: I haz the power, 2017-05-15 16:37:42]

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, 2017-05-15 17:12:43]

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

[commit-bot: I haz the power, 2017-05-15 17:12:45]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jkwang, 2017-05-15 18:42:47]

The CQ bit was checked by jkwang@google.com

[commit-bot: I haz the power, 2017-05-15 18:43:32]

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, 2017-05-15 21:35:39]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494873767154680,
"parent_rev": "9a68250c14a6e3a9053b9bd650db14943764ca95", "commit_rev":
"8e72a1beacc36f4f0e4d9cb3e4404cf8d896d931"}

[commit-bot: I haz the power, 2017-05-15 21:35:54]

Description was changed from

==========
Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=720529
==========

to

==========
Support Coalesced Touch in ppapi

This patch route coalesced events through webplugin interface. The
coalesced touch events will be dispatched to nacl when client request
PP_INPUTEVENT_CLASS_COALESCED_TOUCH.

BUG=720529

Review-Url: https://codereview.chromium.org/2844823002
Cr-Commit-Position: refs/heads/master@{#471909}
Committed:
https://chromium.googlesource.com/chromium/src/+/8e72a1beacc36f4f0e4d9cb3e440...
==========

[commit-bot: I haz the power, 2017-05-15 21:35:56]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/8e72a1beacc36f4f0e4d9cb3e440...