Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1617863002:
Set the correct pressure for pointer events based on force (Closed)
|
Created:
4 years, 11 months ago by Navid Zolghadr Modified:
4 years, 10 months ago CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-api_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet the correct pressure for pointer events based on the force
Set 0.5, 0.0 when any button is pressed, no button is pressed respectively
for the pointer events if the the device is not supporting pressure.
BUG=578869
Committed: https://crrev.com/88114bcec70bd1402e8d25a73062a0b8a6ea92a3
Cr-Commit-Position: refs/heads/master@{#372773}
Patch Set 1 #Patch Set 2 : Add the new test result #Patch Set 3 : Fix Win/OSX compile error #
Total comments: 4
Patch Set 4 : Set the Chrome side #Patch Set 5 : Fix the browser tests #Patch Set 6 : Fix another test #Patch Set 7 : #
Total comments: 2
Patch Set 8 : Adding a comment describing force #Patch Set 9 : Add ASSERT #Patch Set 10 : Fix some tests #Patch Set 11 : Remove a typo #
Total comments: 5
Patch Set 12 : Sadrul's suggestions #Patch Set 13 : Fix the tests #
Total comments: 12
Patch Set 14 : Applying comments #Messages
Total messages: 45 (11 generated)
Description was changed from ========== Set the correct pressure for PEs based on force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ========== to ========== Set the correct pressure for PEs based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ==========
nzolghadr@chromium.org changed reviewers: + mustaq@chromium.org, rbyers@chromium.org
This is the Blink half of the fix for pressure.
Please check the refs to WebPointerProperties.force to make sure the default NaN won't cause any problems. Other than that, I have a few minor comments in tests. https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-event-properties.html (right): https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-event-properties.html:56: shouldBeEqualToNumber("event.pressure", event.buttons ? 0.5 : 0.0); Please add 'pressure' in numericAttributes[] above, and tweak the expected value as in L78. This will keep the ME-vs-PE diffs in one place. https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventManager.cpp (right): https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventManager.cpp:112: pointerEventInit.setPressure(isnan(touchPoint.force()) Please create a helper function to avoid repeats.
I applied all the comments and a small tweak in Chromium side as well not to set the force when the pointer type is mouse. Now it works end to end for mouse. Let me know if this is what you had in mind as well. https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-event-properties.html (right): https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/pointerevents/mouse-pointer-event-properties.html:56: shouldBeEqualToNumber("event.pressure", event.buttons ? 0.5 : 0.0); On 2016/01/25 16:34:29, mustaq wrote: > Please add 'pressure' in numericAttributes[] above, and tweak the expected value > as in L78. This will keep the ME-vs-PE diffs in one place. Done. https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/PointerEventManager.cpp (right): https://codereview.chromium.org/1617863002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/PointerEventManager.cpp:112: pointerEventInit.setPressure(isnan(touchPoint.force()) On 2016/01/25 16:34:29, mustaq wrote: > Please create a helper function to avoid repeats. Done.
https://codereview.chromium.org/1617863002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebPointerProperties.h (right): https://codereview.chromium.org/1617863002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebPointerProperties.h:47: float force; nit: please add a comment describing the semantics here: range [0,1], with NaN meaning not supported.
https://codereview.chromium.org/1617863002/diff/120001/third_party/WebKit/pub... File third_party/WebKit/public/platform/WebPointerProperties.h (right): https://codereview.chromium.org/1617863002/diff/120001/third_party/WebKit/pub... third_party/WebKit/public/platform/WebPointerProperties.h:47: float force; On 2016/01/25 21:32:22, Rick Byers wrote: > nit: please add a comment describing the semantics here: range [0,1], with NaN > meaning not supported. Done.
Thanks, LGTM. Please add a DCHECK at PlatformTouchPoint.force() for !NaN, just to be safe.
LGTM
On 2016/01/26 18:02:56, mustaq wrote: > Thanks, LGTM. > > Please add a DCHECK at PlatformTouchPoint.force() for !NaN, just to be safe. Done.
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, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1617863002/#ps160001 (title: "Add ASSERT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617863002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617863002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
I had to change a few things to pass some of the tests after adding the Assertion for the force() function. ptal.
nzolghadr@chromium.org changed reviewers: + sadrul@chromium.org, tkent@chromium.org
tkent@chromium.org: Please review changes in third_party/WebKit/Source/platform/PlatformTouchPoint.h sadrul@chromium.org: Please review changes in content/browser/renderer_host/web_input_event_aura.cc content/browser/renderer_host/web_input_event_aura_unittest
https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:410: webkit_event.force = event.pointer_details().force(); Can we update the default value for |ui::PointerDetails::force_| instead, so that we can unconditionally set this here. The lower level layer which creates the ui events should be responsible for setting the right value if/when the platform supports it. https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:443: webkit_event.force = event.pointer_details().force(); ditto
Please update the CL title and description to mention PointerEvent instead of 'PE'
On 2016/01/27 18:19:36, Navid Zolghadr wrote: > mailto:tkent@chromium.org: Please review changes in > > third_party/WebKit/Source/platform/PlatformTouchPoint.h That new change LGTM to me (my LGTM is enough to satisfy OWNERS, but the codereview extention doesn't know that because of the use of file:// in WebKit/OWNERS). > mailto:sadrul@chromium.org: Please review changes in > > content/browser/renderer_host/web_input_event_aura.cc > content/browser/renderer_host/web_input_event_aura_unittest
Description was changed from ========== Set the correct pressure for PEs based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ========== to ========== Set the correct pressure for pointer events based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ==========
https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:410: webkit_event.force = event.pointer_details().force(); On 2016/01/27 18:31:44, sadrul wrote: > Can we update the default value for |ui::PointerDetails::force_| instead, so > that we can unconditionally set this here. The lower level layer which creates > the ui events should be responsible for setting the right value if/when the > platform supports it. That would practically add a lot more implications and makes this CL a lot bigger particularly about touch cases. That is why we have it like this to have the changes mostly in Blink for this CL. Later that we add the support for Mac force touch pad I will clean up the Chrome side as well. So I will merge this change as is if you are okay with that.
https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:410: webkit_event.force = event.pointer_details().force(); On 2016/01/27 19:55:48, Navid Zolghadr wrote: > On 2016/01/27 18:31:44, sadrul wrote: > > Can we update the default value for |ui::PointerDetails::force_| instead, so > > that we can unconditionally set this here. The lower level layer which creates > > the ui events should be responsible for setting the right value if/when the > > platform supports it. > > That would practically add a lot more implications and makes this CL a lot > bigger particularly about touch cases. Can you clarify why you think that would make this CL any bigger? Does it involve anything other than changing the default initialization value of |ui::PointerDetails::force_|? > That is why we have it like this to have > the changes mostly in Blink for this CL. Later that we add the support for Mac > force touch pad I will clean up the Chrome side as well. So I will merge this > change as is if you are okay with that.
On 2016/01/27 20:22:05, sadrul wrote: > https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... > content/browser/renderer_host/web_input_event_aura.cc:410: webkit_event.force = > event.pointer_details().force(); > On 2016/01/27 19:55:48, Navid Zolghadr wrote: > > On 2016/01/27 18:31:44, sadrul wrote: > > > Can we update the default value for |ui::PointerDetails::force_| instead, so > > > that we can unconditionally set this here. The lower level layer which > creates > > > the ui events should be responsible for setting the right value if/when the > > > platform supports it. > > > > That would practically add a lot more implications and makes this CL a lot > > bigger particularly about touch cases. > > Can you clarify why you think that would make this CL any bigger? Does it > involve anything other than changing the default initialization value of > |ui::PointerDetails::force_|? > > > That is why we have it like this to have > > the changes mostly in Blink for this CL. Later that we add the support for Mac > > force touch pad I will clean up the Chrome side as well. So I will merge this > > change as is if you are okay with that. I might be wrong. Let me do the change you are suggesting and upload to see if any tests failing. Hopefully not and it just works :)
I added the changes as per Sadrul's suggestions and also fixed a few tests for that. As far as I tested myself I didn't see any crash or assertion failure playing with Chrome with touch and mouse.
ptal https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:410: webkit_event.force = event.pointer_details().force(); On 2016/01/27 20:22:05, sadrul wrote: > On 2016/01/27 19:55:48, Navid Zolghadr wrote: > > On 2016/01/27 18:31:44, sadrul wrote: > > > Can we update the default value for |ui::PointerDetails::force_| instead, so > > > that we can unconditionally set this here. The lower level layer which > creates > > > the ui events should be responsible for setting the right value if/when the > > > platform supports it. > > > > That would practically add a lot more implications and makes this CL a lot > > bigger particularly about touch cases. > > Can you clarify why you think that would make this CL any bigger? Does it > involve anything other than changing the default initialization value of > |ui::PointerDetails::force_|? > > > That is why we have it like this to have > > the changes mostly in Blink for this CL. Later that we add the support for Mac > > force touch pad I will clean up the Chrome side as well. So I will merge this > > change as is if you are okay with that. > Done.
On 2016/01/28 18:38:38, Navid Zolghadr wrote: > ptal > > https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... > File content/browser/renderer_host/web_input_event_aura.cc (right): > > https://codereview.chromium.org/1617863002/diff/200001/content/browser/render... > content/browser/renderer_host/web_input_event_aura.cc:410: webkit_event.force = > event.pointer_details().force(); > On 2016/01/27 20:22:05, sadrul wrote: > > On 2016/01/27 19:55:48, Navid Zolghadr wrote: > > > On 2016/01/27 18:31:44, sadrul wrote: > > > > Can we update the default value for |ui::PointerDetails::force_| instead, > so > > > > that we can unconditionally set this here. The lower level layer which > > creates > > > > the ui events should be responsible for setting the right value if/when > the > > > > platform supports it. > > > > > > That would practically add a lot more implications and makes this CL a lot > > > bigger particularly about touch cases. > > > > Can you clarify why you think that would make this CL any bigger? Does it > > involve anything other than changing the default initialization value of > > |ui::PointerDetails::force_|? > > > > > That is why we have it like this to have > > > the changes mostly in Blink for this CL. Later that we add the support for > Mac > > > force touch pad I will clean up the Chrome side as well. So I will merge > this > > > change as is if you are okay with that. > > > > Done. Sadrul, could you please have a quick look at this. Thanks
lgtm. Thanks! https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:408: Undo this change. https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... File content/browser/renderer_host/web_input_event_aura_unittest.cc (right): https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... content/browser/renderer_host/web_input_event_aura_unittest.cc:428: EXPECT_TRUE(isnan(webkit_event.force)); std::isnan https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); Can you explain why this change was necessary?
https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... File content/browser/renderer_host/web_input_event_aura.cc (right): https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... content/browser/renderer_host/web_input_event_aura.cc:408: On 2016/01/29 16:30:43, sadrul wrote: > Undo this change. Done. https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... File content/browser/renderer_host/web_input_event_aura_unittest.cc (right): https://codereview.chromium.org/1617863002/diff/240001/content/browser/render... content/browser/renderer_host/web_input_event_aura_unittest.cc:428: EXPECT_TRUE(isnan(webkit_event.force)); On 2016/01/29 16:30:43, sadrul wrote: > std::isnan Done. https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/01/29 16:30:43, sadrul wrote: > Can you explain why this change was necessary? Without this it was failing in one of the ASSERTs.
https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/02/01 15:41:33, Navid Zolghadr wrote: > On 2016/01/29 16:30:43, sadrul wrote: > > Can you explain why this change was necessary? > > Without this it was failing in one of the ASSERTs. Which one?
https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/02/01 16:07:08, sadrul wrote: > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > On 2016/01/29 16:30:43, sadrul wrote: > > > Can you explain why this change was necessary? > > > > Without this it was failing in one of the ASSERTs. > > Which one? The test was this one WebContentsViewAuraTest.RepeatedQuickOverscrollGestures and the ASSERT that was failing is the one that is added in this same CL in PlatformTouchPoint.h file.
https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/02/01 16:21:30, Navid Zolghadr wrote: > On 2016/02/01 16:07:08, sadrul wrote: > > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > > On 2016/01/29 16:30:43, sadrul wrote: > > > > Can you explain why this change was necessary? > > > > > > Without this it was failing in one of the ASSERTs. > > > > Which one? > > The test was this one WebContentsViewAuraTest.RepeatedQuickOverscrollGestures > and the ASSERT that was failing is the one that is added in this same CL in > PlatformTouchPoint.h file. Interesting. Should PlatformTouchPointBuilder take care of that (like PointerEventManager.cpp does in this CL)? For example, what happens if a touchscreen device does not report the pressure? Do we make sure (somewhere else in the browser-side) that we do not send nan for touch-events to the renderer?
https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/02/01 16:46:40, sadrul wrote: > On 2016/02/01 16:21:30, Navid Zolghadr wrote: > > On 2016/02/01 16:07:08, sadrul wrote: > > > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > > > On 2016/01/29 16:30:43, sadrul wrote: > > > > > Can you explain why this change was necessary? > > > > > > > > Without this it was failing in one of the ASSERTs. > > > > > > Which one? > > > > The test was this one WebContentsViewAuraTest.RepeatedQuickOverscrollGestures > > and the ASSERT that was failing is the one that is added in this same CL in > > PlatformTouchPoint.h file. > > Interesting. Should PlatformTouchPointBuilder take care of that (like > PointerEventManager.cpp does in this CL)? > > For example, what happens if a touchscreen device does not report the pressure? > Do we make sure (somewhere else in the browser-side) that we do not send nan for > touch-events to the renderer? Not sure how you like PlatformTouchPointBuilder takes care of that. Do you mean when it is NAN just sets it to 0? As far as I followed we always set the pressure for touch events as we set the default value in the API call to 0. But ideally when we there is a touch screen with no pressure support we should somehow pass that value down to Blink and Blink removes this ASSERT. However, I suspect in that time we need to change a few other places in Blink as well as in some places that Blink uses pressure for touch events it doesn't check for NAN and this last point was the main reason for adding this ASSERT in this CL to catch anything that goes wrong at this stage in debug builds but eventually I believe the NAN value of touch events should be passed to Blink as well.
https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/02/01 16:53:14, Navid Zolghadr wrote: > On 2016/02/01 16:46:40, sadrul wrote: > > On 2016/02/01 16:21:30, Navid Zolghadr wrote: > > > On 2016/02/01 16:07:08, sadrul wrote: > > > > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > > > > On 2016/01/29 16:30:43, sadrul wrote: > > > > > > Can you explain why this change was necessary? > > > > > > > > > > Without this it was failing in one of the ASSERTs. > > > > > > > > Which one? > > > > > > The test was this one > WebContentsViewAuraTest.RepeatedQuickOverscrollGestures > > > and the ASSERT that was failing is the one that is added in this same CL in > > > PlatformTouchPoint.h file. > > > > Interesting. Should PlatformTouchPointBuilder take care of that (like > > PointerEventManager.cpp does in this CL)? > > > > For example, what happens if a touchscreen device does not report the > pressure? > > Do we make sure (somewhere else in the browser-side) that we do not send nan > for > > touch-events to the renderer? > > Not sure how you like PlatformTouchPointBuilder takes care of that. Do you mean > when it is NAN just sets it to 0? I was thinking 0.5? Or is that not the default pressure to use for touch-events? > As far as I followed we always set the > pressure for touch events as we set the default value in the API call to 0. But > ideally when we there is a touch screen with no pressure support we should > somehow pass that value down to Blink and Blink removes this ASSERT. However, I > suspect in that time we need to change a few other places in Blink as well as in > some places that Blink uses pressure for touch events it doesn't check for NAN > and this last point was the main reason for adding this ASSERT in this CL to > catch anything that goes wrong at this stage in debug builds but eventually I > believe the NAN value of touch events should be passed to Blink as well. I think the handling of the special value (NAN in this case) should happen in one place. I think if it happens during Web*Event/Point -> Platform*Event/Point conversion, that would be best, since that means none of the other parts of the system needs to know about this NAN -> 0.5 conversion (and I believe you can remove the changes in PointerEventManager too, since the Platform*Event will already have the correct value). But if the default value for pressure for TouchEvent is not the same as for PlatformEvent, then that wouldn't work, of course, and what you have here is fine in that case.
https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... File ui/events/test/event_generator.cc (right): https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... ui/events/test/event_generator.cc:395: Dispatch(&release); On 2016/02/01 17:06:34, sadrul wrote: > On 2016/02/01 16:53:14, Navid Zolghadr wrote: > > On 2016/02/01 16:46:40, sadrul wrote: > > > On 2016/02/01 16:21:30, Navid Zolghadr wrote: > > > > On 2016/02/01 16:07:08, sadrul wrote: > > > > > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > > > > > On 2016/01/29 16:30:43, sadrul wrote: > > > > > > > Can you explain why this change was necessary? > > > > > > > > > > > > Without this it was failing in one of the ASSERTs. > > > > > > > > > > Which one? > > > > > > > > The test was this one > > WebContentsViewAuraTest.RepeatedQuickOverscrollGestures > > > > and the ASSERT that was failing is the one that is added in this same CL > in > > > > PlatformTouchPoint.h file. > > > > > > Interesting. Should PlatformTouchPointBuilder take care of that (like > > > PointerEventManager.cpp does in this CL)? > > > > > > For example, what happens if a touchscreen device does not report the > > pressure? > > > Do we make sure (somewhere else in the browser-side) that we do not send nan > > for > > > touch-events to the renderer? > > > > Not sure how you like PlatformTouchPointBuilder takes care of that. Do you > mean > > when it is NAN just sets it to 0? > > I was thinking 0.5? Or is that not the default pressure to use for touch-events? > > > As far as I followed we always set the > > pressure for touch events as we set the default value in the API call to 0. > But > > ideally when we there is a touch screen with no pressure support we should > > somehow pass that value down to Blink and Blink removes this ASSERT. However, > I > > suspect in that time we need to change a few other places in Blink as well as > in > > some places that Blink uses pressure for touch events it doesn't check for NAN > > and this last point was the main reason for adding this ASSERT in this CL to > > catch anything that goes wrong at this stage in debug builds but eventually I > > believe the NAN value of touch events should be passed to Blink as well. > > I think the handling of the special value (NAN in this case) should happen in > one place. I think if it happens during Web*Event/Point -> Platform*Event/Point > conversion, that would be best, since that means none of the other parts of the > system needs to know about this NAN -> 0.5 conversion (and I believe you can > remove the changes in PointerEventManager too, since the Platform*Event will > already have the correct value). But if the default value for pressure for > TouchEvent is not the same as for PlatformEvent, then that wouldn't work, of > course, and what you have here is fine in that case. I believe Mustaq can give a better answer here but as far as I know this NAN->0.5 is related to pointer event spec only and we need this conversion for whenever we want to send the pointer event equivalent of the events. So I believe having this as pointer event specific change in PointerEventManager is fine with you. Right?
On 2016/02/01 17:54:49, Navid Zolghadr wrote: > https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... > File ui/events/test/event_generator.cc (right): > > https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... > ui/events/test/event_generator.cc:395: Dispatch(&release); > On 2016/02/01 17:06:34, sadrul wrote: > > On 2016/02/01 16:53:14, Navid Zolghadr wrote: > > > On 2016/02/01 16:46:40, sadrul wrote: > > > > On 2016/02/01 16:21:30, Navid Zolghadr wrote: > > > > > On 2016/02/01 16:07:08, sadrul wrote: > > > > > > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > > > > > > On 2016/01/29 16:30:43, sadrul wrote: > > > > > > > > Can you explain why this change was necessary? > > > > > > > > > > > > > > Without this it was failing in one of the ASSERTs. > > > > > > > > > > > > Which one? > > > > > > > > > > The test was this one > > > WebContentsViewAuraTest.RepeatedQuickOverscrollGestures > > > > > and the ASSERT that was failing is the one that is added in this same CL > > in > > > > > PlatformTouchPoint.h file. > > > > > > > > Interesting. Should PlatformTouchPointBuilder take care of that (like > > > > PointerEventManager.cpp does in this CL)? > > > > > > > > For example, what happens if a touchscreen device does not report the > > > pressure? > > > > Do we make sure (somewhere else in the browser-side) that we do not send > nan > > > for > > > > touch-events to the renderer? > > > > > > Not sure how you like PlatformTouchPointBuilder takes care of that. Do you > > mean > > > when it is NAN just sets it to 0? > > > > I was thinking 0.5? Or is that not the default pressure to use for > touch-events? > > > > > As far as I followed we always set the > > > pressure for touch events as we set the default value in the API call to 0. > > But > > > ideally when we there is a touch screen with no pressure support we should > > > somehow pass that value down to Blink and Blink removes this ASSERT. > However, > > I > > > suspect in that time we need to change a few other places in Blink as well > as > > in > > > some places that Blink uses pressure for touch events it doesn't check for > NAN > > > and this last point was the main reason for adding this ASSERT in this CL to > > > catch anything that goes wrong at this stage in debug builds but eventually > I > > > believe the NAN value of touch events should be passed to Blink as well. > > > > I think the handling of the special value (NAN in this case) should happen in > > one place. I think if it happens during Web*Event/Point -> > Platform*Event/Point > > conversion, that would be best, since that means none of the other parts of > the > > system needs to know about this NAN -> 0.5 conversion (and I believe you can > > remove the changes in PointerEventManager too, since the Platform*Event will > > already have the correct value). But if the default value for pressure for > > TouchEvent is not the same as for PlatformEvent, then that wouldn't work, of > > course, and what you have here is fine in that case. > > I believe Mustaq can give a better answer here but as far as I know this > NAN->0.5 is related to pointer event spec only and we need this conversion for > whenever we want to send the pointer event equivalent of the events. So I > believe having this as pointer event specific change in PointerEventManager is > fine with you. Right? Yes.
On 2016/02/01 17:56:49, sadrul wrote: > On 2016/02/01 17:54:49, Navid Zolghadr wrote: > > > https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... > > File ui/events/test/event_generator.cc (right): > > > > > https://codereview.chromium.org/1617863002/diff/240001/ui/events/test/event_g... > > ui/events/test/event_generator.cc:395: Dispatch(&release); > > On 2016/02/01 17:06:34, sadrul wrote: > > > On 2016/02/01 16:53:14, Navid Zolghadr wrote: > > > > On 2016/02/01 16:46:40, sadrul wrote: > > > > > On 2016/02/01 16:21:30, Navid Zolghadr wrote: > > > > > > On 2016/02/01 16:07:08, sadrul wrote: > > > > > > > On 2016/02/01 15:41:33, Navid Zolghadr wrote: > > > > > > > > On 2016/01/29 16:30:43, sadrul wrote: > > > > > > > > > Can you explain why this change was necessary? > > > > > > > > > > > > > > > > Without this it was failing in one of the ASSERTs. > > > > > > > > > > > > > > Which one? > > > > > > > > > > > > The test was this one > > > > WebContentsViewAuraTest.RepeatedQuickOverscrollGestures > > > > > > and the ASSERT that was failing is the one that is added in this same > CL > > > in > > > > > > PlatformTouchPoint.h file. > > > > > > > > > > Interesting. Should PlatformTouchPointBuilder take care of that (like > > > > > PointerEventManager.cpp does in this CL)? > > > > > > > > > > For example, what happens if a touchscreen device does not report the > > > > pressure? > > > > > Do we make sure (somewhere else in the browser-side) that we do not send > > nan > > > > for > > > > > touch-events to the renderer? > > > > > > > > Not sure how you like PlatformTouchPointBuilder takes care of that. Do you > > > mean > > > > when it is NAN just sets it to 0? > > > > > > I was thinking 0.5? Or is that not the default pressure to use for > > touch-events? > > > > > > > As far as I followed we always set the > > > > pressure for touch events as we set the default value in the API call to > 0. > > > But > > > > ideally when we there is a touch screen with no pressure support we should > > > > somehow pass that value down to Blink and Blink removes this ASSERT. > > However, > > > I > > > > suspect in that time we need to change a few other places in Blink as well > > as > > > in > > > > some places that Blink uses pressure for touch events it doesn't check for > > NAN > > > > and this last point was the main reason for adding this ASSERT in this CL > to > > > > catch anything that goes wrong at this stage in debug builds but > eventually > > I > > > > believe the NAN value of touch events should be passed to Blink as well. > > > > > > I think the handling of the special value (NAN in this case) should happen > in > > > one place. I think if it happens during Web*Event/Point -> > > Platform*Event/Point > > > conversion, that would be best, since that means none of the other parts of > > the > > > system needs to know about this NAN -> 0.5 conversion (and I believe you can > > > remove the changes in PointerEventManager too, since the Platform*Event will > > > already have the correct value). But if the default value for pressure for > > > TouchEvent is not the same as for PlatformEvent, then that wouldn't work, of > > > course, and what you have here is fine in that case. > > > > I believe Mustaq can give a better answer here but as far as I know this > > NAN->0.5 is related to pointer event spec only and we need this conversion for > > whenever we want to send the pointer event equivalent of the events. So I > > believe having this as pointer event specific change in PointerEventManager is > > fine with you. Right? > > Yes. I asked for the ASSERT in PlatformTouchPoint.force() to prevent passing NaNs into TouchEvents, apologies if that caused the confusion here. What we wanted in this CL is to be able to identify unknown force values when firing PointerEvents, so that we can replace them to spec specific default later on. We assumed we will gradually have platform-specific APIs in chromium to complete the support for unknown force. I think at this point, we all agreed to keep unset WebPointerProperties.force as NaN, right? I guess it then makes sense to defer the remaining plumbing to separate CLs, including fixing touch-force defaults.
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, rbyers@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1617863002/#ps260001 (title: "Applying comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617863002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617863002/260001
Message was sent while issue was closed.
Description was changed from ========== Set the correct pressure for pointer events based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ========== to ========== Set the correct pressure for pointer events based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Set the correct pressure for pointer events based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 ========== to ========== Set the correct pressure for pointer events based on the force Set 0.5, 0.0 when any button is pressed, no button is pressed respectively for the pointer events if the the device is not supporting pressure. BUG=578869 Committed: https://crrev.com/88114bcec70bd1402e8d25a73062a0b8a6ea92a3 Cr-Commit-Position: refs/heads/master@{#372773} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/88114bcec70bd1402e8d25a73062a0b8a6ea92a3 Cr-Commit-Position: refs/heads/master@{#372773} |
