|
|
Created:
3 years, 11 months ago by Navid Zolghadr Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews, lanwei Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix 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
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Only do the movementx/y got move events #Patch Set 4 : Move bookkeeping of movementX/Y to the browser #Patch Set 5 : Adding wpt automation instructions #Patch Set 6 : Update test instructions #Patch Set 7 : Rebase #Patch Set 8 : Wrong push! #Patch Set 9 : Push the correct commit again #Patch Set 10 : Initialize m_frameScale in PointerEventFactoryTest #
Total comments: 16
Patch Set 11 : Rebase #Patch Set 12 : Fix movementX/Y attributes for pointer events #Patch Set 13 : Move the logic to InputRouterImpl #
Total comments: 9
Patch Set 14 : Rename to match Chromium names #Patch Set 15 : Rebase #Patch Set 16 : Fix MultiPointTouchPress test due to sending pressed twice #Patch Set 17 : Fix the MultiPointTouchPress on Android #
Total comments: 2
Patch Set 18 : Wrap ForwardTouchEventWithLatencyInfo to always reset points #Messages
Total messages: 117 (74 generated)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org
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?
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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).
nzolghadr@chromium.org changed reviewers: + rbyers@chromium.org
rbyers@chromium.org: Please review changes in third_party/WebKit/Source/core/events/PointerEventFactory.*
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.
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.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix movementX/Y attributes for pointer events BUG=678258 ========== to ========== Fix movementX/Y attributes for pointer events BUG=678258 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> 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.
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?
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.
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.
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?
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
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. The corresponding wpt test also landed.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ronniewaynemcc@gmail.com changed reviewers: + RonnieWayneMcC@gmail.com
nzolghadr@chromium.org changed reviewers: + ronniewaynemcc@gmail.com, sadrul@chromium.org - RonnieWayneMcC@gmail.com
sadrul@chromium.org: Please review changes in content/browser/renderer_host/* rbyers@chromium.org: Please review changes in third_party/WebKit/*
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
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.
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?
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?
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.
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.
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.
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?
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?
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
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.
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 ^
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 ==========
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 ==========
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.
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_|?
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 ==========
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 ==========
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 ==========
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 ==========
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nzolghadr@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
nzolghadr@chromium.org changed reviewers: + tdresser@chromium.org
Tim, can you take a look at these files: content/common/input/*
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.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
LGTM, thanks.
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by nzolghadr@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 340001, "attempt_start_ts": 1486756913233550, "parent_rev": "2b355b10cfd06d3a0e4b5fc563a3c9ee2f4e4d79", "commit_rev": "2fe7948da152f6d65725173f0ec1fa916e3a016c"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/2fe7948da152f6d65725173f0ec1... |