Fix movementX/Y attributes for touch pointer events

Fix movementX/Y attributes for touch pointer events

Keep track of previous coordinates of touch points and calculate the deltas
and store them inside the WebTouchPoints. Since pointerevents have
movementX/Y attributes which they expose to js this CL calculates the
correct values for movementX/Y of touch pointerevents.

BUG=678258
Review-Url: https://codereview.chromium.org/2624783002
Cr-Commit-Position: refs/heads/master@{#449714}
Committed: https://chromium.googlesource.com/chromium/src/+/2fe7948da152f6d65725173f0ec1fa916e3a016c

[Navid Zolghadr, 2017-01-10 17:19:06]

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

[commit-bot: I haz the power, 2017-01-10 17:19:30]

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-01-10 17:21:01]

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

[commit-bot: I haz the power, 2017-01-10 17:21:02]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Navid Zolghadr, 2017-01-10 17:25:26]

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

[commit-bot: I haz the power, 2017-01-10 17:25:50]

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

[Navid Zolghadr, 2017-01-10 17:25:57]

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

[commit-bot: I haz the power, 2017-01-10 19:54:55]

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

[commit-bot: I haz the power, 2017-01-10 19:54:55]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2017-01-11 19:03:38]

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

[mustaq, 2017-01-11 19:55:16]

We need a test. A W3C test is needed anyway, but a new check in an existing
layout test should be easy too.

https://codereview.chromium.org/2624783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2624783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:126:
touchPoint.state() == PlatformTouchPoint::TouchPressed) {
Did you mean TouchReleased instead?

[Navid Zolghadr, 2017-01-16 18:56:36]

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

[commit-bot: I haz the power, 2017-01-16 18:56:43]

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

[Navid Zolghadr, 2017-01-16 18:57:23]

Is it okay to just have a w3c test for this? I prefer to start removing our own
layout tests sometime.

https://codereview.chromium.org/2624783002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/2624783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:126:
touchPoint.state() == PlatformTouchPoint::TouchPressed) {
On 2017/01/11 19:55:16, mustaq wrote:
> Did you mean TouchReleased instead?

I did mean release. But after the issue that I filed with pointerlock I kind of
got convinced to only set these for move events. So now I only check for move
here.

[Navid Zolghadr, 2017-01-16 18:57:39]

ptal.

[commit-bot: I haz the power, 2017-01-16 20:14:33]

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

[commit-bot: I haz the power, 2017-01-16 20:14:33]

Dry run: This issue passed the CQ dry run.

[mustaq, 2017-01-16 20:32:58]

On 2017/01/16 18:57:39, Navid Zolghadr wrote:
> ptal.

We should merge this to M56, so we should be extra careful about the outcome.
At least adding a test for mouse, in mouse-pointer-event-properties, makes
sense since it's easy.

[mustaq, 2017-01-16 21:08:25]

On 2017/01/16 20:32:58, mustaq wrote:
> On 2017/01/16 18:57:39, Navid Zolghadr wrote:
> > ptal.
> 
> We should merge this to M56, so we should be extra careful about the outcome.
> At least adding a test for mouse, in mouse-pointer-event-properties, makes
> sense since it's easy.

LGTM w/o a test since we won't merge. I was wrong, the diff with Edge doesn't
qualify for push to stable.

But please add wpt test soon (and keep the bug open until then).

[Navid Zolghadr, 2017-01-16 21:10:24]

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

[Navid Zolghadr, 2017-01-16 21:10:25]

rbyers@chromium.org: Please review changes in 

third_party/WebKit/Source/core/events/PointerEventFactory.*

[Rick Byers, 2017-01-16 22:00:41]

On 2017/01/16 21:08:25, mustaq wrote:
> On 2017/01/16 20:32:58, mustaq wrote:
> > On 2017/01/16 18:57:39, Navid Zolghadr wrote:
> > > ptal.
> > 
> > We should merge this to M56, so we should be extra careful about the
outcome.
> > At least adding a test for mouse, in mouse-pointer-event-properties, makes
> > sense since it's easy.
> 
> LGTM w/o a test since we won't merge. I was wrong, the diff with Edge doesn't
> qualify for push to stable.
> 
> But please add wpt test soon (and keep the bug open until then).

We're ready to start dogfooding the WPT export process.  Want to just include
your WPT test change (edit to LayoutTests/imported/wpt) in this CL?  Alternately
include a link to the WPT pull request.

In particular I think we need a test for the case of the pointer crossing an
iframe boundary - I'm concerned that may be broken with the code as is.

Otherwise the code looks reasonable to me - point to a passing test somewhere
for the above and I'll approve.

[Rick Byers, 2017-01-16 22:00:41]

On 2017/01/16 21:08:25, mustaq wrote:
> On 2017/01/16 20:32:58, mustaq wrote:
> > On 2017/01/16 18:57:39, Navid Zolghadr wrote:
> > > ptal.
> > 
> > We should merge this to M56, so we should be extra careful about the
outcome.
> > At least adding a test for mouse, in mouse-pointer-event-properties, makes
> > sense since it's easy.
> 
> LGTM w/o a test since we won't merge. I was wrong, the diff with Edge doesn't
> qualify for push to stable.
> 
> But please add wpt test soon (and keep the bug open until then).

We're ready to start dogfooding the WPT export process.  Want to just include
your WPT test change (edit to LayoutTests/imported/wpt) in this CL?  Alternately
include a link to the WPT pull request.

In particular I think we need a test for the case of the pointer crossing an
iframe boundary - I'm concerned that may be broken with the code as is.

Otherwise the code looks reasonable to me - point to a passing test somewhere
for the above and I'll approve.

[Navid Zolghadr, 2017-01-19 20:45:09]

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

[commit-bot: I haz the power, 2017-01-19 20:45:41]

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

[Navid Zolghadr, 2017-01-19 20:54:39]

ptal.
I had to move the whole logic back to the browser to be able to address the
problem.
It seems that for the new test to be added to wpt we need to update the
manifest.json as well. But we don't upstream that. So I believe adding a new
test through our upstreaming process doesn't quite work. Anyhow, I pushed the
test
https://github.com/w3c/web-platform-tests/pull/4571
through a separate PR and added the automation instructions here.

[Navid Zolghadr, 2017-01-19 22:21:43]

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

[commit-bot: I haz the power, 2017-01-19 22:22:13]

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

[Navid Zolghadr, 2017-01-19 22:24:25]

Description was changed from

==========
Fix movementX/Y attributes for pointer events

BUG=678258
==========

to

==========
Fix movementX/Y attributes for pointer events

BUG=678258
==========

[commit-bot: I haz the power, 2017-01-20 00:49:09]

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

[commit-bot: I haz the power, 2017-01-20 00:49:10]

Dry run: This issue passed the CQ dry run.

[Rick Byers, 2017-01-20 15:14:15]

> It seems that for the new test to be added to wpt we need to update the
> manifest.json as well. But we don't upstream that. So I believe adding a new
> test through our upstreaming process doesn't quite work.

Really?  I don't understand the role of the MANIFEST.json file myself.  Please
file a bug assigned to jeffcarp that describes what you saw.  Presumably we've
only ever used the export process so far for modifying existing tests.

[Rick Byers, 2017-01-20 15:19:11]

On 2017/01/19 20:54:39, Navid Zolghadr wrote:
> ptal.
> I had to move the whole logic back to the browser to be able to address the
> problem.

I haven't reviewed the code in detail yet, but for the record and others here:
Navid and I talked about the problem of tracking movementX/Y inside the renderer
for the case of out-of-process iframes.  Without doing this in the browser it
would be ugly to avoid cases where you see a large movement value when
entering/existing an out-of-process iframe and probably really tricky to avoid
(the less important) problem of some gaps in the movement values at the moment
of crossing the iframe boundary.  

Ultimately we concluded that the way movementX/Y was already being calculated
for mouse events was the right pattern, and we should just promote that to a
WebPointerProperties thing instead of a WebMouseEvent-specific thing.

dtapuska, mustaq, thoughts?

[dtapuska, 2017-01-20 15:25:52]

On 2017/01/20 15:19:11, Rick Byers wrote:
> On 2017/01/19 20:54:39, Navid Zolghadr wrote:
> > ptal.
> > I had to move the whole logic back to the browser to be able to address the
> > problem.
> 
> I haven't reviewed the code in detail yet, but for the record and others here:
> Navid and I talked about the problem of tracking movementX/Y inside the
renderer
> for the case of out-of-process iframes.  Without doing this in the browser it
> would be ugly to avoid cases where you see a large movement value when
> entering/existing an out-of-process iframe and probably really tricky to avoid
> (the less important) problem of some gaps in the movement values at the moment
> of crossing the iframe boundary.  
> 
> Ultimately we concluded that the way movementX/Y was already being calculated
> for mouse events was the right pattern, and we should just promote that to a
> WebPointerProperties thing instead of a WebMouseEvent-specific thing.
> 
> dtapuska, mustaq, thoughts?

yup OOPIF is certainly a concern here. Ya it can probably land on
WebPointerProperties fine.

[Navid Zolghadr, 2017-01-20 15:39:09]

On 2017/01/20 15:14:15, Rick Byers wrote:
> > It seems that for the new test to be added to wpt we need to update the
> > manifest.json as well. But we don't upstream that. So I believe adding a new
> > test through our upstreaming process doesn't quite work.
> 
> Really?  I don't understand the role of the MANIFEST.json file myself.  Please
> file a bug assigned to jeffcarp that describes what you saw.  Presumably we've
> only ever used the export process so far for modifying existing tests.

Yeah. I was talking to Quinten about it and he told me there is no support for
upstreaming MANIFEST.json.
Basically, right now we only look at the tests listed in MANIFEST.json and
consider them as a test. So if you just add
a new html file under wpt it won't run with run-webkit-tests until you also add
it to MANIFEST.json and there is
also a script for it.

[mustaq, 2017-01-20 15:42:13]

On 2017/01/20 15:19:11, Rick Byers wrote:
> On 2017/01/19 20:54:39, Navid Zolghadr wrote:
> > ptal.
> > I had to move the whole logic back to the browser to be able to address the
> > problem.
> 
> I haven't reviewed the code in detail yet, but for the record and others here:
> Navid and I talked about the problem of tracking movementX/Y inside the
renderer
> for the case of out-of-process iframes.  Without doing this in the browser it
> would be ugly to avoid cases where you see a large movement value when
> entering/existing an out-of-process iframe and probably really tricky to avoid
> (the less important) problem of some gaps in the movement values at the moment
> of crossing the iframe boundary.  
> 
> Ultimately we concluded that the way movementX/Y was already being calculated
> for mouse events was the right pattern, and we should just promote that to a
> WebPointerProperties thing instead of a WebMouseEvent-specific thing.
> 
> dtapuska, mustaq, thoughts?

Moving movementDelta to WebPointerProperties would look cleaner, thanks.

I agree we can ignore the large delta problem for now. But should we change the
spec to define the deltas more precisely in boundary crossing case? E.g. by 
specifying that the "last mouse event" used in offset calculation is "last in
current browsing context" or "last in top level browsing context". Thoughts?

[mustaq, 2017-01-20 15:44:32]

On 2017/01/20 15:42:13, mustaq wrote:
> On 2017/01/20 15:19:11, Rick Byers wrote:
> > On 2017/01/19 20:54:39, Navid Zolghadr wrote:
> > > ptal.
> > > I had to move the whole logic back to the browser to be able to address
the
> > > problem.
> > 
> > I haven't reviewed the code in detail yet, but for the record and others
here:
> > Navid and I talked about the problem of tracking movementX/Y inside the
> renderer
> > for the case of out-of-process iframes.  Without doing this in the browser
it
> > would be ugly to avoid cases where you see a large movement value when
> > entering/existing an out-of-process iframe and probably really tricky to
avoid
> > (the less important) problem of some gaps in the movement values at the
moment
> > of crossing the iframe boundary.  
> > 
> > Ultimately we concluded that the way movementX/Y was already being
calculated
> > for mouse events was the right pattern, and we should just promote that to a
> > WebPointerProperties thing instead of a WebMouseEvent-specific thing.
> > 
> > dtapuska, mustaq, thoughts?
> 
> Moving movementDelta to WebPointerProperties would look cleaner, thanks.
> 
> I agree we can ignore the large delta problem for now. But should we change
the
> spec to define the deltas more precisely in boundary crossing case? E.g. by 
> specifying that the "last mouse event" used in offset calculation is "last in
> current browsing context" or "last in top level browsing context". Thoughts?

Let's discuss it in the spec issue: https://github.com/w3c/pointerlock/issues/16

[Navid Zolghadr, 2017-02-03 15:17:58]

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

[commit-bot: I haz the power, 2017-02-03 15:18:23]

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-02-03 15:20:49]

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

[commit-bot: I haz the power, 2017-02-03 15:20:50]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Navid Zolghadr, 2017-02-03 16:42:11]

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

[commit-bot: I haz the power, 2017-02-03 16:42:41]

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

[Navid Zolghadr, 2017-02-03 16:47:33]

ptal.
The corresponding wpt test also landed.

[commit-bot: I haz the power, 2017-02-03 18:19:33]

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

[commit-bot: I haz the power, 2017-02-03 18:19:34]

Dry run: 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_...)

[Navid Zolghadr, 2017-02-04 19:05:01]

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

[commit-bot: I haz the power, 2017-02-04 19:05:16]

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-02-04 21:06:04]

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

[commit-bot: I haz the power, 2017-02-04 21:06:05]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_TIMED_OUT,
no build URL)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no build
URL)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_TIMED_OUT, no
build URL)
  win_clang on master.tryserver.chromium.win (JOB_TIMED_OUT, no build URL)

[Navid Zolghadr, 2017-02-05 19:58:54]

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

[commit-bot: I haz the power, 2017-02-05 19:59:07]

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-02-05 21:30:04]

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

[commit-bot: I haz the power, 2017-02-05 21:30:06]

Dry run: This issue passed the CQ dry run.

[RonnieWayneMcC, 2017-02-06 21:51:10]

ronniewaynemcc@gmail.com changed reviewers:
+ RonnieWayneMcC@gmail.com

[Navid Zolghadr, 2017-02-06 21:53:41]

nzolghadr@chromium.org changed reviewers:
+ ronniewaynemcc@gmail.com, sadrul@chromium.org
- RonnieWayneMcC@gmail.com

[Navid Zolghadr, 2017-02-06 21:53:42]

sadrul@chromium.org: Please review changes in 
content/browser/renderer_host/*

rbyers@chromium.org: Please review changes in 
third_party/WebKit/*

[Rick Byers, 2017-02-06 22:00:53]

Looks great, thanks!  Just one question.

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:826: 
nit: no reason to remove this line?

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:489: std::map<int,
gfx::Point> global_touch_position_;
Is there a reason to put this here instead of in
RenderWidgetHostViewEventHandler where the corresponding fields for mouse
already are?  In general RenderWidgetHostViewBase is already pretty big, so the
more we can keep event-related-state / processing to
RenderWidgetHostViewEventHandler the cleaner IMHO.

https://codereview.chromium.org/2624783002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/2624783002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebPointerProperties.h:86: int movementX;
nit: add a comment saying what co-ordinate space this is in

[Navid Zolghadr, 2017-02-06 22:40:41]

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:489: std::map<int,
gfx::Point> global_touch_position_;
On 2017/02/06 22:00:53, Rick Byers wrote:
> Is there a reason to put this here instead of in
> RenderWidgetHostViewEventHandler where the corresponding fields for mouse
> already are?  In general RenderWidgetHostViewBase is already pretty big, so
the
> more we can keep event-related-state / processing to
> RenderWidgetHostViewEventHandler the cleaner IMHO.

I asked about this from jonross@. He mentioned RenderWidgetHostViewEventHandler
was created to abstract the logic of Aura to be used in mus later on. So those
two classes will share RenderWidgetHostViewEventHandler and not the Android one.
He also mentioned that the class doesn't seem to be necessary after some
discussions on how mus should integrate in. Anyhow RenderWidgetHostViewAndroid 
doesn't have any instance of RenderWidgetHostViewEventHandler. That is why I put
it in the main class.

[sadrul, 2017-02-07 03:48:30]

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:513: auto&
touchPoint = event->touches[i];
touch_point

Perhaps you could make this a pointer? (I think we don't use non-const refs in
chromium code very often)

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:515: gfx::Point&
lastPosition = global_touch_position_[touchPoint.id];
const gfx::Point& last_position

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:489: std::map<int,
gfx::Point> global_touch_position_;
On 2017/02/06 22:40:41, Navid Zolghadr wrote:
> On 2017/02/06 22:00:53, Rick Byers wrote:
> > Is there a reason to put this here instead of in
> > RenderWidgetHostViewEventHandler where the corresponding fields for mouse
> > already are?  In general RenderWidgetHostViewBase is already pretty big, so
> the
> > more we can keep event-related-state / processing to
> > RenderWidgetHostViewEventHandler the cleaner IMHO.
> 
> I asked about this from jonross@. He mentioned
RenderWidgetHostViewEventHandler
> was created to abstract the logic of Aura to be used in mus later on. So those
> two classes will share RenderWidgetHostViewEventHandler and not the Android
one.
> He also mentioned that the class doesn't seem to be necessary after some
> discussions on how mus should integrate in. Anyhow RenderWidgetHostViewAndroid

> doesn't have any instance of RenderWidgetHostViewEventHandler. That is why I
put
> it in the main class.

Can this live in InputRouterImpl instead? Ideally, each platform wouldn't have
to remember to explicitly update the fields. Putting this code in
InputRouterImpl will make sure the fields are updated correctly on all
platforms.

https://codereview.chromium.org/2624783002/diff/180001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.cc (right):

https://codereview.chromium.org/2624783002/diff/180001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.cc:340: }
Mind adding a test for this in blink_event_util_unittest.cc?

[Navid Zolghadr, 2017-02-07 15:43:05]

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:489: std::map<int,
gfx::Point> global_touch_position_;
On 2017/02/07 03:48:30, sadrul wrote:
> On 2017/02/06 22:40:41, Navid Zolghadr wrote:
> > On 2017/02/06 22:00:53, Rick Byers wrote:
> > > Is there a reason to put this here instead of in
> > > RenderWidgetHostViewEventHandler where the corresponding fields for mouse
> > > already are?  In general RenderWidgetHostViewBase is already pretty big,
so
> > the
> > > more we can keep event-related-state / processing to
> > > RenderWidgetHostViewEventHandler the cleaner IMHO.
> > 
> > I asked about this from jonross@. He mentioned
> RenderWidgetHostViewEventHandler
> > was created to abstract the logic of Aura to be used in mus later on. So
those
> > two classes will share RenderWidgetHostViewEventHandler and not the Android
> one.
> > He also mentioned that the class doesn't seem to be necessary after some
> > discussions on how mus should integrate in. Anyhow
RenderWidgetHostViewAndroid
> 
> > doesn't have any instance of RenderWidgetHostViewEventHandler. That is why I
> put
> > it in the main class.
> 
> Can this live in InputRouterImpl instead? Ideally, each platform wouldn't have
> to remember to explicitly update the fields. Putting this code in
> InputRouterImpl will make sure the fields are updated correctly on all
> platforms.

The reason I added it here is mostly because I wanted to keep it in (almost) the
same as setting movementX/Y properties for mouse which is done in
render_widget_host_view_event_handler.cc. Do you think that can also be moved to
InputRouterImpl then?

[Navid Zolghadr, 2017-02-07 15:58:51]

https://codereview.chromium.org/2624783002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/2624783002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebPointerProperties.h:86: int movementX;
On 2017/02/06 22:00:53, Rick Byers wrote:
> nit: add a comment saying what co-ordinate space this is in

I'm not sure there is any answer for this. Dave added a m_frameScale variable in
WebInputEvent which scales this coordinates in movementInRootFrame function
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/WebMo....
I added the same in WebTouchEvent in this CL.

So these will be transformed when they get to blink. So I believe they will have
different meaning? Also I don't know what different coordinate spaces are called
before or after transformations.

[sadrul, 2017-02-08 06:59:09]

On 2017/02/07 15:43:05, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_base.h (right):
> 
>
https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_base.h:489:
std::map<int,
> gfx::Point> global_touch_position_;
> On 2017/02/07 03:48:30, sadrul wrote:
> > On 2017/02/06 22:40:41, Navid Zolghadr wrote:
> > > On 2017/02/06 22:00:53, Rick Byers wrote:
> > > > Is there a reason to put this here instead of in
> > > > RenderWidgetHostViewEventHandler where the corresponding fields for
mouse
> > > > already are?  In general RenderWidgetHostViewBase is already pretty big,
> so
> > > the
> > > > more we can keep event-related-state / processing to
> > > > RenderWidgetHostViewEventHandler the cleaner IMHO.
> > > 
> > > I asked about this from jonross@. He mentioned
> > RenderWidgetHostViewEventHandler
> > > was created to abstract the logic of Aura to be used in mus later on. So
> those
> > > two classes will share RenderWidgetHostViewEventHandler and not the
Android
> > one.
> > > He also mentioned that the class doesn't seem to be necessary after some
> > > discussions on how mus should integrate in. Anyhow
> RenderWidgetHostViewAndroid
> > 
> > > doesn't have any instance of RenderWidgetHostViewEventHandler. That is why
I
> > put
> > > it in the main class.
> > 
> > Can this live in InputRouterImpl instead? Ideally, each platform wouldn't
have
> > to remember to explicitly update the fields. Putting this code in
> > InputRouterImpl will make sure the fields are updated correctly on all
> > platforms.
> 
> The reason I added it here is mostly because I wanted to keep it in (almost)
the
> same as setting movementX/Y properties for mouse which is done in
> render_widget_host_view_event_handler.cc. Do you think that can also be moved
to
> InputRouterImpl then?

If it'd be useful for non-aura platforms, then sure. That would be a good change
to make.

[sadrul, 2017-02-08 07:01:52]

Also, please make sure to update the CL description to include a little more
detail. I understand that the linked BUG has more info, but having to always go
to crbug (especially when doing code archaeology with git log) is not ideal.

[Navid Zolghadr, 2017-02-08 17:18:49]

ptal.

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (left):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:826: 
On 2017/02/06 22:00:53, Rick Byers wrote:
> nit: no reason to remove this line?

Done.

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:513: auto&
touchPoint = event->touches[i];
On 2017/02/07 03:48:30, sadrul wrote:
> touch_point
> 
> Perhaps you could make this a pointer? (I think we don't use non-const refs in
> chromium code very often)

Done.

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:515: gfx::Point&
lastPosition = global_touch_position_[touchPoint.id];
On 2017/02/07 03:48:30, sadrul wrote:
> const gfx::Point& last_position

Done.

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.h (right):

https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.h:489: std::map<int,
gfx::Point> global_touch_position_;
On 2017/02/07 15:43:05, Navid Zolghadr wrote:
> On 2017/02/07 03:48:30, sadrul wrote:
> > On 2017/02/06 22:40:41, Navid Zolghadr wrote:
> > > On 2017/02/06 22:00:53, Rick Byers wrote:
> > > > Is there a reason to put this here instead of in
> > > > RenderWidgetHostViewEventHandler where the corresponding fields for
mouse
> > > > already are?  In general RenderWidgetHostViewBase is already pretty big,
> so
> > > the
> > > > more we can keep event-related-state / processing to
> > > > RenderWidgetHostViewEventHandler the cleaner IMHO.
> > > 
> > > I asked about this from jonross@. He mentioned
> > RenderWidgetHostViewEventHandler
> > > was created to abstract the logic of Aura to be used in mus later on. So
> those
> > > two classes will share RenderWidgetHostViewEventHandler and not the
Android
> > one.
> > > He also mentioned that the class doesn't seem to be necessary after some
> > > discussions on how mus should integrate in. Anyhow
> RenderWidgetHostViewAndroid
> > 
> > > doesn't have any instance of RenderWidgetHostViewEventHandler. That is why
I
> > put
> > > it in the main class.
> > 
> > Can this live in InputRouterImpl instead? Ideally, each platform wouldn't
have
> > to remember to explicitly update the fields. Putting this code in
> > InputRouterImpl will make sure the fields are updated correctly on all
> > platforms.
> 
> The reason I added it here is mostly because I wanted to keep it in (almost)
the
> same as setting movementX/Y properties for mouse which is done in
> render_widget_host_view_event_handler.cc. Do you think that can also be moved
to
> InputRouterImpl then?

Done. I haven't moved the mouse one yet. But I'll keep that in mind to do it in
a later round of clean up when we add the mouse support to Android as well so
that logic would fully apply to all the platforms.

https://codereview.chromium.org/2624783002/diff/180001/ui/events/blink/blink_...
File ui/events/blink/blink_event_util.cc (right):

https://codereview.chromium.org/2624783002/diff/180001/ui/events/blink/blink_...
ui/events/blink/blink_event_util.cc:340: }
On 2017/02/07 03:48:30, sadrul wrote:
> Mind adding a test for this in blink_event_util_unittest.cc?

Done.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:187: void
InputRouterImpl::SendTouchEvent(TouchEventWithLatencyInfo touch_event) {
InputRouterImpl particularly was a better place for it as it also enabled the
logic for the touch emulation which the previous logic didn't. There is only
this problem of the event being const ref. I'm not sure just copying it like
this was a very ideal solution. What do you suggest?

[Rick Byers, 2017-02-08 21:27:47]

On 2017/02/06 22:40:41, Navid Zolghadr wrote:
>
https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
> File content/browser/renderer_host/render_widget_host_view_base.h (right):
> 
>
https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
> content/browser/renderer_host/render_widget_host_view_base.h:489:
std::map<int,
> gfx::Point> global_touch_position_;
> On 2017/02/06 22:00:53, Rick Byers wrote:
> > Is there a reason to put this here instead of in
> > RenderWidgetHostViewEventHandler where the corresponding fields for mouse
> > already are?  In general RenderWidgetHostViewBase is already pretty big, so
> the
> > more we can keep event-related-state / processing to
> > RenderWidgetHostViewEventHandler the cleaner IMHO.
> 
> I asked about this from jonross@. He mentioned
RenderWidgetHostViewEventHandler
> was created to abstract the logic of Aura to be used in mus later on. So those
> two classes will share RenderWidgetHostViewEventHandler and not the Android
one.
> He also mentioned that the class doesn't seem to be necessary after some
> discussions on how mus should integrate in. Anyhow RenderWidgetHostViewAndroid

> doesn't have any instance of RenderWidgetHostViewEventHandler. That is why I
put
> it in the main class.

Oh interesting, so movementX/Y is presumably broken for Mouse on Android today? 
Is there a bug tracking that?

[Rick Byers, 2017-02-08 21:45:14]

LGTM

https://codereview.chromium.org/2624783002/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebPointerProperties.h (right):

https://codereview.chromium.org/2624783002/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebPointerProperties.h:86: int movementX;
On 2017/02/07 15:58:51, Navid Zolghadr wrote:
> On 2017/02/06 22:00:53, Rick Byers wrote:
> > nit: add a comment saying what co-ordinate space this is in
> 
> I'm not sure there is any answer for this. Dave added a m_frameScale variable
in
> WebInputEvent which scales this coordinates in movementInRootFrame function
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/WebMo....
> I added the same in WebTouchEvent in this CL.
> 
> So these will be transformed when they get to blink. So I believe they will
have
> different meaning? Also I don't know what different coordinate spaces are
called
> before or after transformations.

Ugh, right.  That makes it hard to reason about all these co-ordinates
(movementX/Y is the least of our potential confusion here).  Ok ignore this for
now.  Maybe later we can add some comments better describing the co-ordinate
system for all of the WebTouchEvent/WebMouseEvent co-ordinates.  FWIW the
official documentation is here:
https://www.chromium.org/developers/design-documents/blink-coordinate-spaces

[Navid Zolghadr, 2017-02-08 21:52:23]

On 2017/02/08 21:27:47, Rick Byers wrote:
> On 2017/02/06 22:40:41, Navid Zolghadr wrote:
> >
>
https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
> > File content/browser/renderer_host/render_widget_host_view_base.h (right):
> > 
> >
>
https://codereview.chromium.org/2624783002/diff/180001/content/browser/render...
> > content/browser/renderer_host/render_widget_host_view_base.h:489:
> std::map<int,
> > gfx::Point> global_touch_position_;
> > On 2017/02/06 22:00:53, Rick Byers wrote:
> > > Is there a reason to put this here instead of in
> > > RenderWidgetHostViewEventHandler where the corresponding fields for mouse
> > > already are?  In general RenderWidgetHostViewBase is already pretty big,
so
> > the
> > > more we can keep event-related-state / processing to
> > > RenderWidgetHostViewEventHandler the cleaner IMHO.
> > 
> > I asked about this from jonross@. He mentioned
> RenderWidgetHostViewEventHandler
> > was created to abstract the logic of Aura to be used in mus later on. So
those
> > two classes will share RenderWidgetHostViewEventHandler and not the Android
> one.
> > He also mentioned that the class doesn't seem to be necessary after some
> > discussions on how mus should integrate in. Anyhow
RenderWidgetHostViewAndroid
> 
> > doesn't have any instance of RenderWidgetHostViewEventHandler. That is why I
> put
> > it in the main class.
> 
> Oh interesting, so movementX/Y is presumably broken for Mouse on Android
today? 
> Is there a bug tracking that?

At the time of me writing this code it wasn't supported at least. I just asked
Mustaq and he mentioned he enabled it again on the tip of the tree.
I filed https://crbug.com/690168 for the work related to that.

[sadrul, 2017-02-09 01:23:30]

On 2017/02/08 07:01:52, sadrul wrote:
> Also, please make sure to update the CL description to include a little more
> detail. I understand that the linked BUG has more info, but having to always
go
> to crbug (especially when doing code archaeology with git log) is not ideal.

ping re ^

[Navid Zolghadr, 2017-02-09 01:30:55]

Description was changed from

==========
Fix movementX/Y attributes for pointer events

BUG=678258
==========

to

==========
Keep track of global X/Y of touch points and calculate the deltas and set them
inside the WebTouchPoints. 
Since pointerevents also have movementX/Y attributes they expose these
this information and this CL calculates the correct values for movementX/Y of
touchointerevents.

BUG=678258
==========

[Navid Zolghadr, 2017-02-09 01:32:02]

Description was changed from

==========
Keep track of global X/Y of touch points and calculate the deltas and set them
inside the WebTouchPoints. 
Since pointerevents also have movementX/Y attributes they expose these
this information and this CL calculates the correct values for movementX/Y of
touchointerevents.

BUG=678258
==========

to

==========
Keeping track of coordinates of touch points and calculating the deltas and
storing
them inside the WebTouchPoints. 
Since pointerevents also have movementX/Y attributes they expose these
information to js and this CL calculates the correct values for movementX/Y
of touch pointerevents.

BUG=678258
==========

[Navid Zolghadr, 2017-02-09 01:32:23]

On 2017/02/09 01:23:30, sadrul wrote:
> On 2017/02/08 07:01:52, sadrul wrote:
> > Also, please make sure to update the CL description to include a little more
> > detail. I understand that the linked BUG has more info, but having to always
> go
> > to crbug (especially when doing code archaeology with git log) is not ideal.
> 
> ping re ^

Sorry I missed it. Updated now.

[sadrul, 2017-02-09 03:11:34]

Thank you for updating the CL description! Still some comments on that: please
note that the CL title (or 'subject') is not actually included in the commit
log. So you will notice that the CL descriptions usually include the CL title
(or 'subject'), followed by a blank line, followed by the more detailed
description.

As a sidenote, I think chromium does not enforce a specific style for commit
logs. But please consider following http://chris.beams.io/posts/git-commit/ as a
guideline.

Having said all that, I have some nits. lgtm with those addressed.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
File content/browser/renderer_host/input/input_router.h (right):

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router.h:38: virtual void
SendTouchEvent(TouchEventWithLatencyInfo touch_event) = 0;
I suggest keep this a const-ref, and make a copy in the implementation in
InputRouterImpl::SendTouchEvent() instead.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:187: void
InputRouterImpl::SendTouchEvent(TouchEventWithLatencyInfo touch_event) {
On 2017/02/08 17:18:49, Navid Zolghadr wrote:
> InputRouterImpl particularly was a better place for it as it also enabled the
> logic for the touch emulation which the previous logic didn't. There is only
> this problem of the event being const ref. I'm not sure just copying it like
> this was a very ideal solution. What do you suggest?

(I left a comment in input_router.h)

I think the interface should still take a const-ref. The implementation here
should make a copy.

LegacyTouchEventQueue::CoalescedWebTouchEvent makes a copy anyway. So I don't
think that's going to be a problem.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:643:
blink::WebTouchPoint* touchPoint = &event->touches[i];
touch_point (that's the style used in non-blink chromium code)

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:645: const gfx::Point&
lastPosition = global_touch_position_[touchPoint->id];
last_position

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:658:
global_touch_position_[touchPoint->id] = gfx::Point(
Do you want to DCHECK that |touch_point->id| is not already in
|global_touch_position_|?

[Navid Zolghadr, 2017-02-09 03:21:39]

Description was changed from

==========
Keeping track of coordinates of touch points and calculating the deltas and
storing
them inside the WebTouchPoints. 
Since pointerevents also have movementX/Y attributes they expose these
information to js and this CL calculates the correct values for movementX/Y
of touch pointerevents.

BUG=678258
==========

to

==========
Fix movementX/Y attributes for touch pointer events

Keeping track of coordinates of touch points and calculating the deltas and
storing them inside the WebTouchPoints. 
Since pointerevents also have movementX/Y attributes they expose these
information to js and this CL calculates the correct values for movementX/Y
of touch pointerevents.

BUG=678258
==========

[Navid Zolghadr, 2017-02-09 03:24:15]

Description was changed from

==========
Fix movementX/Y attributes for touch pointer events

Keeping track of coordinates of touch points and calculating the deltas and
storing them inside the WebTouchPoints. 
Since pointerevents also have movementX/Y attributes they expose these
information to js and this CL calculates the correct values for movementX/Y
of touch pointerevents.

BUG=678258
==========

to

==========
Fix movementX/Y attributes for touch pointer events

Keeping track of coordinates of touch points and calculating the deltas and
storing them inside the WebTouchPoints. 
Since pointerevents have movementX/Y attributes which they expose them
to js this CL calculates the correct values for movementX/Y of touch
pointerevents.

BUG=678258
==========

[Navid Zolghadr, 2017-02-09 03:37:00]

Description was changed from

==========
Fix movementX/Y attributes for touch pointer events

Keeping track of coordinates of touch points and calculating the deltas and
storing them inside the WebTouchPoints. 
Since pointerevents have movementX/Y attributes which they expose them
to js this CL calculates the correct values for movementX/Y of touch
pointerevents.

BUG=678258
==========

to

==========
Fix movementX/Y attributes for touch pointer events

Keep track of previous coordinates of touch points and calculate the deltas
and store them inside the WebTouchPoints. 
Since pointerevents have movementX/Y attributes which they expose to js
this CL calculates the correct values for movementX/Y of touch
pointerevents.

BUG=678258
==========

[Navid Zolghadr, 2017-02-09 03:37:37]

Description was changed from

==========
Fix movementX/Y attributes for touch pointer events

Keep track of previous coordinates of touch points and calculate the deltas
and store them inside the WebTouchPoints. 
Since pointerevents have movementX/Y attributes which they expose to js
this CL calculates the correct values for movementX/Y of touch
pointerevents.

BUG=678258
==========

to

==========
Fix movementX/Y attributes for touch pointer events

Keep track of previous coordinates of touch points and calculate the deltas
and store them inside the WebTouchPoints. Since pointerevents have
movementX/Y attributes which they expose to js this CL calculates the
correct values for movementX/Y of touch pointerevents.

BUG=678258
==========

[Navid Zolghadr, 2017-02-09 15:30:50]

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

[commit-bot: I haz the power, 2017-02-09 15:31:11]

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-02-09 15:33:29]

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

[commit-bot: I haz the power, 2017-02-09 15:33:30]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Navid Zolghadr, 2017-02-09 16:21:45]

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

[commit-bot: I haz the power, 2017-02-09 16:22:04]

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

[Navid Zolghadr, 2017-02-09 16:35:08]

Great read regarding the commit message. Hopefully I applied all the tips now.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
File content/browser/renderer_host/input/input_router_impl.cc (right):

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:643:
blink::WebTouchPoint* touchPoint = &event->touches[i];
On 2017/02/09 03:11:34, sadrul wrote:
> touch_point (that's the style used in non-blink chromium code)

Done.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:645: const gfx::Point&
lastPosition = global_touch_position_[touchPoint->id];
On 2017/02/09 03:11:34, sadrul wrote:
> last_position

Done.

https://codereview.chromium.org/2624783002/diff/240001/content/browser/render...
content/browser/renderer_host/input/input_router_impl.cc:658:
global_touch_position_[touchPoint->id] = gfx::Point(
On 2017/02/09 03:11:34, sadrul wrote:
> Do you want to DCHECK that |touch_point->id| is not already in
> |global_touch_position_|?

Done.

[commit-bot: I haz the power, 2017-02-09 17:09:08]

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

[commit-bot: I haz the power, 2017-02-09 17:09:09]

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

[Navid Zolghadr, 2017-02-09 18:04:13]

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

[commit-bot: I haz the power, 2017-02-09 18:04:51]

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-02-09 20:38:39]

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

[commit-bot: I haz the power, 2017-02-09 20:38:41]

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

[Navid Zolghadr, 2017-02-09 21:15:10]

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

[commit-bot: I haz the power, 2017-02-09 21:15:50]

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-02-09 23:03:17]

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

[commit-bot: I haz the power, 2017-02-09 23:03:18]

Dry run: This issue passed the CQ dry run.

[Navid Zolghadr, 2017-02-10 05:44:33]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2017-02-10 05:44:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org,
sadrul@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/2624783002/#ps320001
(title: "Fix the MultiPointTouchPress on Android")

[commit-bot: I haz the power, 2017-02-10 05:44:52]

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-02-10 05:51:52]

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

[commit-bot: I haz the power, 2017-02-10 05:51:54]

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

[Navid Zolghadr, 2017-02-10 06:28:28]

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

[Navid Zolghadr, 2017-02-10 06:28:29]

Tim, can you take a look at these files:

content/common/input/*

[tdresser, 2017-02-10 13:09:07]

https://codereview.chromium.org/2624783002/diff/320001/content/browser/render...
File content/browser/renderer_host/input/touch_input_browsertest.cc (right):

https://codereview.chromium.org/2624783002/diff/320001/content/browser/render...
content/browser/renderer_host/input/touch_input_browsertest.cc:216:
touch.StalePoint(id);
touch.ResetPoints(), and StalePoint isn't needed.
It might be worth wrapping ForwardTouchEventWithLatencyInfo in a method which
calls ResetPoints afterwards, we really should be calling it after every time we
forward the event.

[Navid Zolghadr, 2017-02-10 15:49:03]

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

[commit-bot: I haz the power, 2017-02-10 15:49:27]

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

[Navid Zolghadr, 2017-02-10 16:02:09]

ptal.

https://codereview.chromium.org/2624783002/diff/320001/content/browser/render...
File content/browser/renderer_host/input/touch_input_browsertest.cc (right):

https://codereview.chromium.org/2624783002/diff/320001/content/browser/render...
content/browser/renderer_host/input/touch_input_browsertest.cc:216:
touch.StalePoint(id);
On 2017/02/10 13:09:07, tdresser wrote:
> touch.ResetPoints(), and StalePoint isn't needed.
> It might be worth wrapping ForwardTouchEventWithLatencyInfo in a method which
> calls ResetPoints afterwards, we really should be calling it after every time
we
> forward the event.

Done.

[tdresser, 2017-02-10 16:04:13]

LGTM, thanks.

[commit-bot: I haz the power, 2017-02-10 18:35:58]

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

[commit-bot: I haz the power, 2017-02-10 18:36:00]

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

[Navid Zolghadr, 2017-02-10 20:01:53]

The CQ bit was checked by nzolghadr@chromium.org

[Navid Zolghadr, 2017-02-10 20:01:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org,
rbyers@chromium.org, mustaq@chromium.org
Link to the patchset: https://codereview.chromium.org/2624783002/#ps340001
(title: "Wrap ForwardTouchEventWithLatencyInfo to always reset points")

[commit-bot: I haz the power, 2017-02-10 20:02:43]

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-02-10 20:14:21]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1486756913233550,
"parent_rev": "2b355b10cfd06d3a0e4b5fc563a3c9ee2f4e4d79", "commit_rev":
"2fe7948da152f6d65725173f0ec1fa916e3a016c"}

[commit-bot: I haz the power, 2017-02-10 20:15:23]

Description was changed from

==========
Fix movementX/Y attributes for touch pointer events

Keep track of previous coordinates of touch points and calculate the deltas
and store them inside the WebTouchPoints. Since pointerevents have
movementX/Y attributes which they expose to js this CL calculates the
correct values for movementX/Y of touch pointerevents.

BUG=678258
==========

to

==========
Fix movementX/Y attributes for touch pointer events

Keep track of previous coordinates of touch points and calculate the deltas
and store them inside the WebTouchPoints. Since pointerevents have
movementX/Y attributes which they expose to js this CL calculates the
correct values for movementX/Y of touch pointerevents.

BUG=678258

Review-Url: https://codereview.chromium.org/2624783002
Cr-Commit-Position: refs/heads/master@{#449714}
Committed:
https://chromium.googlesource.com/chromium/src/+/2fe7948da152f6d65725173f0ec1...
==========

[commit-bot: I haz the power, 2017-02-10 20:15:29]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/2fe7948da152f6d65725173f0ec1...