Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
800163005:
Fixed ui::TouchEvent rotation angle out-of-bound issue. (Closed)
DescriptionFixed ui::TouchEvent rotation angle out-of-bound issue.
The rotation angle in ui::TouchEvent should lie within 0 and 90 as per
the spec:
https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html
We have seen cases where the angle we got from hardware is not within
the limits. This CL keeps the angle value sane.
BUG=438256
Committed: https://crrev.com/2598c69a3c184095cdcc911b24cb47930b66c5ec
Cr-Commit-Position: refs/heads/master@{#312447}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Removed few unsed setters from ui::TouchEvent #
Total comments: 9
Patch Set 3 : Tweaked ui:TouchEvent angle range, added unittests. #
Total comments: 6
Patch Set 4 : Added tests for motion_event_aura angles #
Total comments: 2
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 32 (6 generated)
mustaq@chromium.org changed reviewers: + tdresser@chromium.org
ptal. I haven't checked the actual errors in angle yet, but I think clipping out-of-range angle is a better idea. Warping to the expected range (like what is done now) would cause a 90-deg rotated outcome which is a significant issue for a true ellipse.
On 2015/01/16 20:20:54, mustaq wrote: > ptal. > > I haven't checked the actual errors in angle yet, but I think clipping > out-of-range angle is a better idea. Warping to the expected range (like what is > done now) would cause a 90-deg rotated outcome which is a significant issue for > a true ellipse. We'll want some unit tests here for sure. I don't think we should clip, we should "warp to the expected range" as you've done, but swap the x and y radii when necessary.
https://codereview.chromium.org/800163005/diff/1/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/800163005/diff/1/ui/events/event.h#newcode584 ui/events/event.h:584: // https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html As we're in the ui layer here, I don't think we want to reference web specific requirements. Just state that we ensure the rotation angle is between 0 and 90.
On 2015/01/16 20:29:52, tdresser wrote: > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h > File ui/events/event.h (right): > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h#newcode584 > ui/events/event.h:584: // > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html > As we're in the ui layer here, I don't think we want to reference web specific > requirements. > > Just state that we ensure the rotation angle is between 0 and 90. Do we still have conversion from ui::TouchEvent to blink::WebTouchEvent? If not, maybe we should simply do the cleanup in the MotionEventAura section, ensuring the proper values (touch major, rotation) conform to MotionEvent expectations. The conversion from MotionEvent to WebTouchEvent should just work and produce correct values as long as the MotionEvent implementation conforms to the MotionEvent spec.
On 2015/01/16 20:38:15, jdduke wrote: > On 2015/01/16 20:29:52, tdresser wrote: > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h > > File ui/events/event.h (right): > > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h#newcode584 > > ui/events/event.h:584: // > > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html > > As we're in the ui layer here, I don't think we want to reference web specific > > requirements. > > > > Just state that we ensure the rotation angle is between 0 and 90. > > Do we still have conversion from ui::TouchEvent to blink::WebTouchEvent? If not, > maybe we should simply do the cleanup in the MotionEventAura section, ensuring > the proper values (touch major, rotation) conform to MotionEvent expectations. > The conversion from MotionEvent to WebTouchEvent should just work and produce > correct values as long as the MotionEvent implementation conforms to the > MotionEvent spec. Nope, there's no more conversion from ui::TouchEvent to blink::WebTouchEvent. I'm fine doing the cleanup in MotionEventAura, though I'm not clear on why it's superior to performing the cleanup here. Why do you think it would be better in MotionEventAura? Just because there's a well documented convention we can refer to which indicates why we're performing this cleanup?
On 2015/01/16 20:42:44, tdresser wrote: > On 2015/01/16 20:38:15, jdduke wrote: > > On 2015/01/16 20:29:52, tdresser wrote: > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h > > > File ui/events/event.h (right): > > > > > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h#newcode584 > > > ui/events/event.h:584: // > > > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html > > > As we're in the ui layer here, I don't think we want to reference web > specific > > > requirements. > > > > > > Just state that we ensure the rotation angle is between 0 and 90. > > > > Do we still have conversion from ui::TouchEvent to blink::WebTouchEvent? If > not, > > maybe we should simply do the cleanup in the MotionEventAura section, ensuring > > the proper values (touch major, rotation) conform to MotionEvent expectations. > > The conversion from MotionEvent to WebTouchEvent should just work and produce > > correct values as long as the MotionEvent implementation conforms to the > > MotionEvent spec. > > Nope, there's no more conversion from ui::TouchEvent to blink::WebTouchEvent. > > I'm fine doing the cleanup in MotionEventAura, though I'm not clear on why it's > superior to performing the cleanup here. > Why do you think it would be better in MotionEventAura? Just because there's a > well documented convention we can refer to which indicates why we're performing > this cleanup? For the reasons you stated... we're in the UI layer so imposing web-specific restrictions implicitly feels a bit odd. With the MotionEventAura conversion, you have a single point of validation, rather than validating/fixing all over the place, or dumping cleanup code in accessor functions.
On 2015/01/16 20:49:01, jdduke wrote: > On 2015/01/16 20:42:44, tdresser wrote: > > On 2015/01/16 20:38:15, jdduke wrote: > > > On 2015/01/16 20:29:52, tdresser wrote: > > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h > > > > File ui/events/event.h (right): > > > > > > > > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h#newcode584 > > > > ui/events/event.h:584: // > > > > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html > > > > As we're in the ui layer here, I don't think we want to reference web > > specific > > > > requirements. > > > > > > > > Just state that we ensure the rotation angle is between 0 and 90. > > > > > > Do we still have conversion from ui::TouchEvent to blink::WebTouchEvent? If > > not, > > > maybe we should simply do the cleanup in the MotionEventAura section, > ensuring > > > the proper values (touch major, rotation) conform to MotionEvent > expectations. > > > The conversion from MotionEvent to WebTouchEvent should just work and > produce > > > correct values as long as the MotionEvent implementation conforms to the > > > MotionEvent spec. > > > > Nope, there's no more conversion from ui::TouchEvent to blink::WebTouchEvent. > > > > I'm fine doing the cleanup in MotionEventAura, though I'm not clear on why > it's > > superior to performing the cleanup here. > > Why do you think it would be better in MotionEventAura? Just because there's a > > well documented convention we can refer to which indicates why we're > performing > > this cleanup? > > For the reasons you stated... we're in the UI layer so imposing web-specific > restrictions implicitly feels a bit odd. > > With the MotionEventAura conversion, you have a single point of validation, > rather than validating/fixing all over the place, or dumping cleanup code in > accessor functions. I think that's reasonable, though imposing some restriction on the values in ui::TouchEvent also feels reasonable. We wouldn't be "imposing web specific restrictions", we'd be imposing restrictions (which we already assume) on ui::TouchEvent, which just happen to coincide with the web-specific restrictions. Right now we essentially treat ui::TouchEvent as if the orientation should be between 0 and 90, but we don't enforce it. I think it might make sense to enforce that ui::TouchEvent have a valid orientation, and then DCHECK in MotionEventAura (as well as downstream). I'm fine either way though.
On 2015/01/17 14:28:10, tdresser wrote: > On 2015/01/16 20:49:01, jdduke wrote: > > On 2015/01/16 20:42:44, tdresser wrote: > > > On 2015/01/16 20:38:15, jdduke wrote: > > > > On 2015/01/16 20:29:52, tdresser wrote: > > > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h > > > > > File ui/events/event.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/800163005/diff/1/ui/events/event.h#newcode584 > > > > > ui/events/event.h:584: // > > > > > https://dvcs.w3.org/hg/webevents/raw-file/default/touchevents.html > > > > > As we're in the ui layer here, I don't think we want to reference web > > > specific > > > > > requirements. > > > > > > > > > > Just state that we ensure the rotation angle is between 0 and 90. > > > > > > > > Do we still have conversion from ui::TouchEvent to blink::WebTouchEvent? > If > > > not, > > > > maybe we should simply do the cleanup in the MotionEventAura section, > > ensuring > > > > the proper values (touch major, rotation) conform to MotionEvent > > expectations. > > > > The conversion from MotionEvent to WebTouchEvent should just work and > > produce > > > > correct values as long as the MotionEvent implementation conforms to the > > > > MotionEvent spec. > > > > > > Nope, there's no more conversion from ui::TouchEvent to > blink::WebTouchEvent. > > > > > > I'm fine doing the cleanup in MotionEventAura, though I'm not clear on why > > it's > > > superior to performing the cleanup here. > > > Why do you think it would be better in MotionEventAura? Just because there's > a > > > well documented convention we can refer to which indicates why we're > > performing > > > this cleanup? > > > > For the reasons you stated... we're in the UI layer so imposing web-specific > > restrictions implicitly feels a bit odd. > > > > With the MotionEventAura conversion, you have a single point of validation, > > rather than validating/fixing all over the place, or dumping cleanup code in > > accessor functions. > > I think that's reasonable, though imposing some restriction on the values in > ui::TouchEvent also feels reasonable. We wouldn't be "imposing web specific > restrictions", we'd be imposing restrictions (which we already assume) on > ui::TouchEvent, which just happen to coincide with the web-specific > restrictions. > > Right now we essentially treat ui::TouchEvent as if the orientation should be > between 0 and 90, but we don't enforce it. > I think it might make sense to enforce that ui::TouchEvent have a valid > orientation, and then DCHECK in MotionEventAura (as well as downstream). > > I'm fine either way though. Are you guys ok if I make ui:TouchEvent immutable? The setters (set_radius, set_rotation_angle and set_force) are not used anywhere! This would make it easy to impose restrictions in ui::TouchEvent values w/o any cleanup in accessor functions. If the answer is yes, I'll make ui:TouchEvent angle range [-90, +90] so that we won't need any radius x/y adjustment there. All the non-trivial fixes will be in MotionEventAura.
ptal: made ui::TouchEvent immutable, added a few missing points in the spec, fixed -ve angle handling.
On 2015/01/19 18:24:56, mustaq wrote: > ptal: made ui::TouchEvent immutable, added a few missing points in the spec, > fixed -ve angle handling. It isn't actually immutable, there are other setters. I believe that we do use some of the setters you removed in some tests. I don't think they're widely used though.
https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc#newco... ui/events/event.cc:606: void TouchEvent::fixRotationAngle() { The bounds are [-90, 90], correct? So this should be < and >, not <= and >=? I don't think taking the number mod 90 is correct. Don't you need to swap the major/minor (x/y) axis in this case as well? https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h#newcod... ui/events/event.h:553: // Radius of the X (major) axis of the touch ellipse. 0.0 if unknown. Sometime we should consider renaming to radius_major_ and radius_minor_. https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... ui/events/gestures/motion_event_aura.cc:32: if (rotation_angle_rad < 0) { You might want to clarify why you're doing this, and what assumptions you have about the input. https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... ui/events/gestures/motion_event_aura.cc:34: float tmp = radius_x; Use std::swap.
On 2015/01/20 13:36:40, tdresser wrote: > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc > File ui/events/event.cc (right): > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc#newco... > ui/events/event.cc:606: void TouchEvent::fixRotationAngle() { > The bounds are [-90, 90], correct? So this should be < and >, not <= and >=? > > I don't think taking the number mod 90 is correct. Don't you need to swap the > major/minor (x/y) axis in this case as well? > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h > File ui/events/event.h (right): > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h#newcod... > ui/events/event.h:553: // Radius of the X (major) axis of the touch ellipse. 0.0 > if unknown. > Sometime we should consider renaming to radius_major_ and radius_minor_. > > https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... > File ui/events/gestures/motion_event_aura.cc (right): > > https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... > ui/events/gestures/motion_event_aura.cc:32: if (rotation_angle_rad < 0) { > You might want to clarify why you're doing this, and what assumptions you have > about the input. > > https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... > ui/events/gestures/motion_event_aura.cc:34: float tmp = radius_x; > Use std::swap. Can you add some tests?
On 2015/01/20 13:38:04, tdresser wrote: > On 2015/01/20 13:36:40, tdresser wrote: > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc > > File ui/events/event.cc (right): > > > > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc#newco... > > ui/events/event.cc:606: void TouchEvent::fixRotationAngle() { > > The bounds are [-90, 90], correct? So this should be < and >, not <= and >=? > > > > I don't think taking the number mod 90 is correct. Don't you need to swap the > > major/minor (x/y) axis in this case as well? > > > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h > > File ui/events/event.h (right): > > > > > https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h#newcod... > > ui/events/event.h:553: // Radius of the X (major) axis of the touch ellipse. > 0.0 > > if unknown. > > Sometime we should consider renaming to radius_major_ and radius_minor_. > > > > > https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... > > File ui/events/gestures/motion_event_aura.cc (right): > > > > > https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... > > ui/events/gestures/motion_event_aura.cc:32: if (rotation_angle_rad < 0) { > > You might want to clarify why you're doing this, and what assumptions you have > > about the input. > > > > > https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... > > ui/events/gestures/motion_event_aura.cc:34: float tmp = radius_x; > > Use std::swap. > > Can you add some tests? Done.
ptal https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/event.cc#newco... ui/events/event.cc:606: void TouchEvent::fixRotationAngle() { On 2015/01/20 13:36:39, tdresser wrote: > The bounds are [-90, 90], correct? So this should be < and >, not <= and >=? > > I don't think taking the number mod 90 is correct. Don't you need to swap the > major/minor (x/y) axis in this case as well? Good catch, thanks. I have adjusted the angle range: - to an open interval to avoid having multiple sets of values representing a single ellipse, and - to a positive interval to fix the mod issue. https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h#newcod... ui/events/event.h:553: // Radius of the X (major) axis of the touch ellipse. 0.0 if unknown. On 2015/01/20 13:36:39, tdresser wrote: > Sometime we should consider renaming to radius_major_ and radius_minor_. I think radius_x makes more sense here. We can have the minor axis along radius_x. https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... ui/events/gestures/motion_event_aura.cc:32: if (rotation_angle_rad < 0) { On 2015/01/20 13:36:39, tdresser wrote: > You might want to clarify why you're doing this, and what assumptions you have > about the input. Done. https://codereview.chromium.org/800163005/diff/20001/ui/events/gestures/motio... ui/events/gestures/motion_event_aura.cc:34: float tmp = radius_x; On 2015/01/20 13:36:39, tdresser wrote: > Use std::swap. Done.
https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/800163005/diff/20001/ui/events/event.h#newcod... ui/events/event.h:553: // Radius of the X (major) axis of the touch ellipse. 0.0 if unknown. On 2015/01/20 16:26:46, mustaq wrote: > On 2015/01/20 13:36:39, tdresser wrote: > > Sometime we should consider renaming to radius_major_ and radius_minor_. > > I think radius_x makes more sense here. We can have the minor axis along > radius_x. Acknowledged. https://codereview.chromium.org/800163005/diff/40001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/800163005/diff/40001/ui/events/event.cc#newco... ui/events/event.cc:607: if (rotation_angle_ < 0) Might be easier to just while (rotation_angle_ < 0) rotation_angle += 180; etc. It's extremely unlikely to be more than one iteration. https://codereview.chromium.org/800163005/diff/40001/ui/events/event_unittest.cc File ui/events/event_unittest.cc (right): https://codereview.chromium.org/800163005/diff/40001/ui/events/event_unittest... ui/events/event_unittest.cc:517: TEST(EventTest, TouchEventRotationAngleFixing) { Awesome, thanks. https://codereview.chromium.org/800163005/diff/40001/ui/events/gestures/motio... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/800163005/diff/40001/ui/events/gestures/motio... ui/events/gestures/motion_event_aura.cc:38: // changes from [0, pi) to [0, pi/2). Wouldn't hurt to have a test for this logic.
Patchset #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
ptal https://codereview.chromium.org/800163005/diff/40001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/800163005/diff/40001/ui/events/event.cc#newco... ui/events/event.cc:607: if (rotation_angle_ < 0) On 2015/01/20 16:40:38, tdresser wrote: > Might be easier to just > > while (rotation_angle_ < 0) > rotation_angle += 180; > > etc. It's extremely unlikely to be more than one iteration. > Done but I am not sure if the minor performance gain justifies the code complexity here. I couldn't find a good answer about fmod performance but out-of-bound cases are rare anyways. https://codereview.chromium.org/800163005/diff/40001/ui/events/gestures/motio... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/800163005/diff/40001/ui/events/gestures/motio... ui/events/gestures/motion_event_aura.cc:38: // changes from [0, pi) to [0, pi/2). On 2015/01/20 16:40:38, tdresser wrote: > Wouldn't hurt to have a test for this logic. Done.
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/800163005/diff/40001/ui/events/event.cc File ui/events/event.cc (right): https://codereview.chromium.org/800163005/diff/40001/ui/events/event.cc#newco... ui/events/event.cc:607: if (rotation_angle_ < 0) On 2015/01/21 16:43:10, mustaq wrote: > On 2015/01/20 16:40:38, tdresser wrote: > > Might be easier to just > > > > while (rotation_angle_ < 0) > > rotation_angle += 180; > > > > etc. It's extremely unlikely to be more than one iteration. > > > > Done but I am not sure if the minor performance gain justifies the code > complexity here. I couldn't find a good answer about fmod performance but > out-of-bound cases are rare anyways. Taking my objection back after simplifying the code a bit.
LGTM with nit. https://codereview.chromium.org/800163005/diff/120001/ui/events/gestures/moti... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/800163005/diff/120001/ui/events/gestures/moti... ui/events/gestures/motion_event_aura.cc:40: rotation_angle_rad -= (float) M_PI_2; Use static_cast<float>
On 2015/01/21 17:09:08, tdresser wrote: > LGTM with nit. > > https://codereview.chromium.org/800163005/diff/120001/ui/events/gestures/moti... > File ui/events/gestures/motion_event_aura.cc (right): > > https://codereview.chromium.org/800163005/diff/120001/ui/events/gestures/moti... > ui/events/gestures/motion_event_aura.cc:40: rotation_angle_rad -= (float) > M_PI_2; > Use static_cast<float> You may want to add a bug that those TODO's refer to, to keep them from getting lost.
Created crbug.com/450655 to track unittest cleanup. https://codereview.chromium.org/800163005/diff/120001/ui/events/gestures/moti... File ui/events/gestures/motion_event_aura.cc (right): https://codereview.chromium.org/800163005/diff/120001/ui/events/gestures/moti... ui/events/gestures/motion_event_aura.cc:40: rotation_angle_rad -= (float) M_PI_2; On 2015/01/21 17:09:08, tdresser wrote: > Use static_cast<float> Done.
mustaq@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org: Please review changes in ui/events.
LGTM https://codereview.chromium.org/800163005/diff/140001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/800163005/diff/140001/ui/events/event.h#newco... ui/events/event.h:572: void fixRotationAngle(); FixRotationAngle(). Move this right after 'private:' above touch_id_
https://codereview.chromium.org/800163005/diff/140001/ui/events/event.h File ui/events/event.h (right): https://codereview.chromium.org/800163005/diff/140001/ui/events/event.h#newco... ui/events/event.h:572: void fixRotationAngle(); On 2015/01/21 18:42:45, sadrul wrote: > FixRotationAngle(). Move this right after 'private:' above touch_id_ Done.
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/800163005/160001
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2598c69a3c184095cdcc911b24cb47930b66c5ec Cr-Commit-Position: refs/heads/master@{#312447} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
