Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
788923002:
Touch Events - changedTouches list includes non-changed touch points on Android (Closed)
|
Created:
6 years ago by USE s.singapati at gmail.com Modified:
5 years, 10 months ago Reviewers:
jdduke (slow) CC:
chromium-reviews, darin-cc_chromium.org, jdduke+watch_chromium.org, jam, Zeeshan Qureshi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnsure touchmove changedTouches list only includes moving touches
On Android, WebTouchPoint.state of all touches is set to "StateMoved" thus
included in changedTouches when any touch moves. This patch compares current
touch points position to previous position in TouchMove event and updates state.
This comparision is done after all filtering and potential coalescing
in TouchEventQueue.
BUG=416236
Committed: https://crrev.com/750a80d9cce15010794afcd80f514e67421e2c01
Cr-Commit-Position: refs/heads/master@{#314315}
Patch Set 1 #
Total comments: 6
Patch Set 2 : stores touchevent without latency info and other review comment fixes #
Total comments: 6
Patch Set 3 : Added unit test and fixed review comments #
Total comments: 13
Patch Set 4 : Added CHECK for last sent event and minor fixes. #
Total comments: 4
Patch Set 5 : Relaxed touches state condition for TouchMove events for now #
Total comments: 4
Patch Set 6 : minor changes in comments ONLY. #
Total comments: 2
Patch Set 7 : Fixed trybots errors. #
Total comments: 3
Patch Set 8 : minor change to comments #
Total comments: 2
Patch Set 9 : comparing all properties of touch points to update the state. #
Total comments: 6
Patch Set 10 : Added unit test for checking Pointer states when other than position changed. #
Total comments: 10
Patch Set 11 : Removed new methods in synthetic_web_input_event_builders #Patch Set 12 : Rebased #
Messages
Total messages: 55 (11 generated)
s.singapati@samsung.com changed reviewers: + jdduke@chromium.org, rbyers@chromium.org, zeeshanq@chromium.org
https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:707: TouchEventWithLatencyInfo& touch) { This logic can probably be folded into SendTouchEventImmediately; we already do some bookkeeping there. https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:721: if (current_touchmove_point.state == WebTouchPoint::StateMoved I don't think there are any guarantees that pointers with the same ID will have the same index in the touches array. https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:732: last_sent_touchmove_.reset(new TouchEventWithLatencyInfo(touch)); Do we need to store the LatencyInfo? I think we can just store a TouchEvent? And to avoid unnecessary allocations, we could do something like: if (last_sent_touchmove_) *last_sent_touchmove_ = touch.event; else last_sent_touchmove_.reset(new WebTouchEvent(touch.event)); Alternatively, you could not use a scoped_ptr and implicitly reset the event with last_sent_touchmove_ = WebInputEvent(); then you just check for a valid event type... but maybe that's abusive...
rbyers@chromium.org changed reviewers: - rbyers@chromium.org, zeeshanq@chromium.org
rbyers@chromium.org changed reviewers: + rbyers@chromium.org
Moving Zeeshan and myself from reviewers to CC - Jared is the right reviewer here. We try to avoid redundant review work by either having a single reviewer, or being clear about what files / OWNERS stamps are required from additional reviewers.
Thank you for comments. My answers below. On 2014/12/09 20:30:26, jdduke wrote: > https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/touch_event_queue.cc:707: > TouchEventWithLatencyInfo& touch) { > This logic can probably be folded into SendTouchEventImmediately; we already do > some bookkeeping there. ==> DONE. > https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/touch_event_queue.cc:721: if > (current_touchmove_point.state == WebTouchPoint::StateMoved > I don't think there are any guarantees that pointers with the same ID will have > the same index in the touches array. ==> DONE. Form testing it is found that most of the times (i can not say 100%) pointer ids are same in same index. But i agree that it is not guaranteed. So, to eliminate margin of error, i used nested loops. With current nature of touches it takes O(n(n+1)/2) complexity. And practically there would be few touches on screen at a time, so other techniques like hash table etc is probably not needed. > https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... > content/browser/renderer_host/input/touch_event_queue.cc:732: > last_sent_touchmove_.reset(new TouchEventWithLatencyInfo(touch)); > Do we need to store the LatencyInfo? I think we can just store a TouchEvent? And > to avoid unnecessary allocations, we could do something like: > > if (last_sent_touchmove_) > *last_sent_touchmove_ = touch.event; > else > last_sent_touchmove_.reset(new WebTouchEvent(touch.event)); > > Alternatively, you could not use a scoped_ptr and implicitly reset the event > with > last_sent_touchmove_ = WebInputEvent(); > > then you just check for a valid event type... but maybe that's abusive... ==> DONE:
rbyers@chromium.org changed reviewers: - rbyers@chromium.org
https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:707: TouchEventWithLatencyInfo& touch) { On 2014/12/09 20:30:25, jdduke wrote: > This logic can probably be folded into SendTouchEventImmediately; we already do > some bookkeeping there. Done. https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:721: if (current_touchmove_point.state == WebTouchPoint::StateMoved On 2014/12/09 20:30:25, jdduke wrote: > I don't think there are any guarantees that pointers with the same ID will have > the same index in the touches array. Done. Form testing it is found that most of the times (i can not say 100%) pointer ids are same in same index. But i agree that it is not guaranteed. So, to eliminate margin of error, i used nested loops. With current nature of touches it takes O(n(n+1)/2) complexity. And practically there would be few touches on screen at a time, so other techniques like hash table etc is probably not needed. https://codereview.chromium.org/788923002/diff/1/content/browser/renderer_hos... content/browser/renderer_host/input/touch_event_queue.cc:732: last_sent_touchmove_.reset(new TouchEventWithLatencyInfo(touch)); On 2014/12/09 20:30:25, jdduke wrote: > Do we need to store the LatencyInfo? I think we can just store a TouchEvent? And > to avoid unnecessary allocations, we could do something like: > > if (last_sent_touchmove_) > *last_sent_touchmove_ = touch.event; > else > last_sent_touchmove_.reset(new WebTouchEvent(touch.event)); > > Alternatively, you could not use a scoped_ptr and implicitly reset the event > with > last_sent_touchmove_ = WebInputEvent(); > > then you just check for a valid event type... but maybe that's abusive... Done.
This needs a test (or two) in touch_event_queue_unittest.cc. https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:706: TouchEventWithLatencyInfo& touch) { Nit: Either pass by const ref, or by pointer (the latter for cases when the argument is modified). https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:729: if (fabs(current_touchmove_point.position.x - Can we avoid using the epsilon? If the touch really hasn't been updated, the position should match precisely, I think. https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:744: if (last_sent_touchmove_) Nit: No need for the if, you can just unconditionally reset the scoped_ptr.
Added a unit test. Any need for more testing? Also, Logic has been modified a bit to update touches state correctly in the FIRST NEW TouchMove event (in subsequent set of TouchMoves). In PatchSet#3, it stores any sent touch event instead of just TouchMove event. https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:706: TouchEventWithLatencyInfo& touch) { On 2014/12/15 16:51:08, jdduke wrote: > Nit: Either pass by const ref, or by pointer (the latter for cases when the > argument is modified). Done. https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:729: if (fabs(current_touchmove_point.position.x - On 2014/12/15 16:51:08, jdduke wrote: > Can we avoid using the epsilon? If the touch really hasn't been updated, the > position should match precisely, I think. Done. Did more testing and no problems found. https://codereview.chromium.org/788923002/diff/20001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:744: if (last_sent_touchmove_) On 2014/12/15 16:51:08, jdduke wrote: > Nit: No need for the if, you can just unconditionally reset the scoped_ptr. Done.
https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:720: for (unsigned int i = 0; i < last_sent_touchevent_->touchesLength; ++i) { |CHECK(last_sent_touchevent_);| to make the crash clear. Also maybe a |DCHECK_EQ(last_sent_touchevent_->touchesLength, touch->event.touchesLength);} https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:724: // touches with same id may not have same index in touches array Nit: Capitalize 'Touches' and end comment with period. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:730: last_touch_point.position.y) Nit: Braces around this if body because the condition is multiline. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2254: MoveTouchPoints(2, 3, 3, 3, 4, 4); I would just remove "MoveTouchPoints(2, 3, 3, 3, 4, 4);". https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2257: // Receive an ACK for the last TouchPress event Why defer this ack? It's confusing to perform it here instead of above with the other press acks. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2269: MoveTouchPoints(0, 1.1, 1, 1, 2, 20.001); Why move all of them? Just do MoveTouchPoint(3, 4.1, 4.1);
https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2254: MoveTouchPoints(2, 3, 3, 3, 4, 4); On 2015/01/06 17:57:29, jdduke wrote: > I would just remove "MoveTouchPoints(2, 3, 3, 3, 4, 4);". Actually for unit tests, with SyntheticWebTouchEvent::MovePoint() implementation, only those explicitly specified touches would have state as "WebTouchPoint::StateMoved", otherwise no change in state of touches. This behavior is different in CreateWebTouchPoint() in web_input_event_util.cc. SO, to reproduce the ORIGINAL problem with unit tests we need to call MovePoint() for all touches. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2257: // Receive an ACK for the last TouchPress event On 2015/01/06 17:57:29, jdduke wrote: > Why defer this ack? It's confusing to perform it here instead of above with the > other press acks. Continuing from above comment, those 4 Touchmoves need to be potentially coalesced and sent as one TouchMove. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2269: MoveTouchPoints(0, 1.1, 1, 1, 2, 20.001); On 2015/01/06 17:57:29, jdduke wrote: > Why move all of them? Just do > > MoveTouchPoint(3, 4.1, 4.1); Same as 1st comment.
https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2254: MoveTouchPoints(2, 3, 3, 3, 4, 4); On 2015/01/07 16:08:39, s.singapati wrote: > On 2015/01/06 17:57:29, jdduke wrote: > > I would just remove "MoveTouchPoints(2, 3, 3, 3, 4, 4);". > > Actually for unit tests, with SyntheticWebTouchEvent::MovePoint() > implementation, only those explicitly specified touches would have state as > "WebTouchPoint::StateMoved", otherwise no change in state of touches. This > behavior is different in CreateWebTouchPoint() in web_input_event_util.cc. SO, > to reproduce the ORIGINAL problem with unit tests we need to call MovePoint() > for all touches. I see, that's unfortunate but probably not worth the trouble of addressing now, thanks for the explanation.
In current (baseline) behavior, It is possible that TouchMove event is dispatched to targets even though NONE of the touches position is really changed, but touch state would be "StateMoved" in ALL touches (Ref: Blink's EventHandler::handleTouchEvent). In this case (TouchMove event comes from platform and NONE of the touches really moved), With this patch, touches state will become "StateStationary" and Blink's EventHandler filters this and do not dispatch the event. This returns with swallowedEvent as false. When DCHECK_IS_ON, There will be a crash from TouchEventStreamValidator::Validate() with error message " No valid touch point corresponding to event type: TouchMove". Any ideas how to deal with this? https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:720: for (unsigned int i = 0; i < last_sent_touchevent_->touchesLength; ++i) { On 2015/01/06 17:57:29, jdduke wrote: > |CHECK(last_sent_touchevent_);| to make the crash clear. > > Also maybe a |DCHECK_EQ(last_sent_touchevent_->touchesLength, > touch->event.touchesLength);} CHECK(last_sent_touchevent_) is fine. Or making if (touch->event.type == WebInputEvent::TouchMove && last_sent_touchevent_) is even safer for not to change current (baseline) behavior allowing event to be sent instead of crashing. Theoretically i think, there should not be TouchMove event without previous sent event. About DCHECK_EQ of touchesLength, when for eg.,3 touches are continuously moving and release one touch, then touchesLength is different in TouchEnd event and next immediate TouchMove event and that is valid. Still in this case it is valid to correct the touches state. Or atleast there is no need to crash. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:724: // touches with same id may not have same index in touches array On 2015/01/06 17:57:29, jdduke wrote: > Nit: Capitalize 'Touches' and end comment with period. Done. https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:730: last_touch_point.position.y) On 2015/01/06 17:57:29, jdduke wrote: > Nit: Braces around this if body because the condition is multiline. Done.
On 2015/01/09 12:28:53, s.singapati wrote: > In current (baseline) behavior, It is possible that TouchMove event is > dispatched to targets even though NONE of the touches position is really > changed, but touch state would be "StateMoved" in ALL touches (Ref: Blink's > EventHandler::handleTouchEvent). > > In this case (TouchMove event comes from platform and NONE of the touches really > moved), With this patch, touches state will become "StateStationary" and Blink's > EventHandler filters this and do not dispatch the event. This returns with > swallowedEvent as false. When DCHECK_IS_ON, There will be a crash from > TouchEventStreamValidator::Validate() with error message " No valid touch point > corresponding to event type: TouchMove". > > Any ideas how to deal with this? > Hmm, good question. I suppose we could check whether at least one point is StateMoved, and if not, we could |PopTouchEventToClient|. Then the question is what kind of ack should we give it... If Blink always reports this as unconsumed, then we could probably do the same. In theory, if nothing moved, there should be no associated gestures. In that case, it may be best to move the filtering logic back into ForwardNextEventToRenderer (but keep last_sent_touchevent_ modification in SendTouchEventImmediately. Hmm, I guess it would be simplest if this filtering logic was in |FilterBeforeForwarding|, but that would require some other changes. Maybe the simplest thing to do do for now would be to leave what you have, relax the condition TouchEventStreamValidator, and I can address that in a follow-up patch. > https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/788923002/diff/40001/content/browser/renderer... > content/browser/renderer_host/input/touch_event_queue.cc:720: for (unsigned int > i = 0; i < last_sent_touchevent_->touchesLength; ++i) { > On 2015/01/06 17:57:29, jdduke wrote: > > |CHECK(last_sent_touchevent_);| to make the crash clear. > > > > Also maybe a |DCHECK_EQ(last_sent_touchevent_->touchesLength, > > touch->event.touchesLength);} > > CHECK(last_sent_touchevent_) is fine. Or making if (touch->event.type == > WebInputEvent::TouchMove && last_sent_touchevent_) is even safer for not to > change current (baseline) behavior allowing event to be sent instead of > crashing. Theoretically i think, there should not be TouchMove event without > previous sent event. > > About DCHECK_EQ of touchesLength, when for eg.,3 touches are continuously moving > and release one touch, then touchesLength is different in TouchEnd event and > next immediate TouchMove event and that is valid. Still in this case it is valid > to correct the touches state. Or atleast there is no need to crash. Of course you're right, this is fine. https://codereview.chromium.org/788923002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:727: if (last_touch_point.id == current_touchmove_point.id) { Style nit: With this many nested levels, let's invert the condition for readability: if (last_touch_point.id != current_touchmove_point.id) continue; ... https://codereview.chromium.org/788923002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:728: if (current_touchmove_point.position.x == last_touch_point.position.x I think this can just be: if (current_touchmove_point.position == last_touch_point.position) touch->event.touches[j].state = WebTouchPoint::Stationary; ...
Touches state condition is relaxed for TouchMove in TouchEventStreamValidator for now. But this should be removed when TouchMove filtering is implemented based on corrected touches state. https://codereview.chromium.org/788923002/diff/60001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:727: if (last_touch_point.id == current_touchmove_point.id) { On 2015/01/09 17:33:05, jdduke wrote: > Style nit: With this many nested levels, let's invert the condition for > readability: > > if (last_touch_point.id != current_touchmove_point.id) > continue; > ... > Done. https://codereview.chromium.org/788923002/diff/60001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.cc:728: if (current_touchmove_point.position.x == last_touch_point.position.x On 2015/01/09 17:33:05, jdduke wrote: > I think this can just be: > > if (current_touchmove_point.position == last_touch_point.position) > touch->event.touches[j].state = WebTouchPoint::Stationary; > ... Done.
lgtm, thanks! https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:171: // Dispatch |touch| to the client. Before dispatching, updates touches state Perhaps "...updates pointer states in touchmove events for pointers that have not changed position.", ending with period. https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:245: // Touch event is saved to compare touches position in next TouchMove event Maybe "... to compare pointer positions for new touchmove events.", again ending with period.
rbyers@: Anything else you want to see here?
On 2015/01/14 16:07:10, jdduke wrote: > rbyers@: Anything else you want to see here? Nope, if you're happy then I'm happy :-). s.singapati@, thanks for doing this - it definitely makes the API make more sense!
Thanks for reviews. I think this is ready to go now. https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer... File content/browser/renderer_host/input/touch_event_queue.h (right): https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:171: // Dispatch |touch| to the client. Before dispatching, updates touches state On 2015/01/14 16:04:33, jdduke wrote: > Perhaps "...updates pointer states in touchmove events for pointers that have > not changed position.", ending with period. Done. https://codereview.chromium.org/788923002/diff/80001/content/browser/renderer... content/browser/renderer_host/input/touch_event_queue.h:245: // Touch event is saved to compare touches position in next TouchMove event On 2015/01/14 16:04:33, jdduke wrote: > Maybe "... to compare pointer positions for new touchmove events.", again ending > with period. Done.
@jdduke : Could you please confirm the commit approval for new patch set?
On 2015/01/20 14:32:49, s.singapati wrote: > @jdduke : Could you please confirm the commit approval for new patch set? Still lgtm, thanks.
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) Try jobs failed on following builders: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/11...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/788923002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2283: MoveTouchPoints(0, 1.1, 1, 1, 2, 20.001); Looks like you simply need to add float modifiers here and below "1.1f", "20.001f", etc...
Needed to have a workaround in TouchEventStreamValidator - InvalidPointStates unit test to fix failing try jobs. https://codereview.chromium.org/788923002/diff/100001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/100001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2283: MoveTouchPoints(0, 1.1, 1, 1, 2, 20.001); On 2015/01/20 18:18:09, jdduke wrote: > Looks like you simply need to add float modifiers here and below "1.1f", > "20.001f", etc... Done.
@jdduke: Could you please check the latest patch set? Can we proceed with commit process? Also, we can work on follow up patch for filtering TouchMoves based on corrected states. Shall we create a new issue already for this?
https://codereview.chromium.org/788923002/diff/120001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/120001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:720: if (current_touchmove_point.position == last_touch_point.position) So, now I'm wondering if we should also check the other properties before marking stationary, e.g., "screenPosition", "radiusX", "radiusY", "rotationAngle" and "force". It seems conceivable that these values might differ despite having the same position... I guess the question is, does "stationary" only refer to position or does it refer to any observable change in the pointer state? If the stationary state is only used to bin the pointer into the |changedTouches| list, then we should check the other properties. rbyers@, tdresser@: Any thoughts here? https://codereview.chromium.org/788923002/diff/120001/content/common/input/to... File content/common/input/touch_event_stream_validator_unittest.cc (right): https://codereview.chromium.org/788923002/diff/120001/content/common/input/to... content/common/input/touch_event_stream_validator_unittest.cc:163: // TODO(): Remove this StateStationary workaround check for Please make this TODO(jdduke), and at the end reference "crbug.com/452032".
https://codereview.chromium.org/788923002/diff/120001/content/common/input/to... File content/common/input/touch_event_stream_validator_unittest.cc (right): https://codereview.chromium.org/788923002/diff/120001/content/common/input/to... content/common/input/touch_event_stream_validator_unittest.cc:163: // TODO(): Remove this StateStationary workaround check for On 2015/01/26 16:18:13, jdduke wrote: > Please make this TODO(jdduke), and at the end reference "crbug.com/452032". Done.
https://codereview.chromium.org/788923002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:720: if (current_touchmove_point.position == last_touch_point.position) Yeah, for correctness, I think we'll need to compare all properties that might have changed, and only modify the state if they all match (maybe create a helper function to do that, |MarkPointStationaryIfUnchanged(last_touch_point, ¤t_touchmove_point);| or if (!HasPointChanged(last_touch_point, current_touchmove_point) current_touchmove_point->state = WebTouchPoint::StateStationary; or something like that.
https://codereview.chromium.org/788923002/diff/140001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/140001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:720: if (current_touchmove_point.position == last_touch_point.position) On 2015/01/27 17:29:54, jdduke wrote: > Yeah, for correctness, I think we'll need to compare all properties that might > have changed, and only modify the state if they all match (maybe create a helper > function to do that, > > |MarkPointStationaryIfUnchanged(last_touch_point, ¤t_touchmove_point);| > > or > > if (!HasPointChanged(last_touch_point, current_touchmove_point) > current_touchmove_point->state = WebTouchPoint::StateStationary; > > or something like that. Done.
Thanks, just a couple things. https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:737: const WebTouchPoint& current_point) const { Nit: This should be a free function in the anonymous namespace at the top of the file (below |OutsideApplicationSlopRegion(...)|). https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:745: } else { Nit: Ditch the else condition, just end with return false. https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2259: TEST_F(TouchEventQueueTest, TouchMoveTouchesState) { Could you also add a test (with just one pointer) to verify that the pointer state doesn't become stationary when properties beside position change?
On 2015/01/28 16:18:39, jdduke wrote: > Thanks, just a couple things. > > https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... > File content/browser/renderer_host/input/touch_event_queue.cc (right): > > https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... > content/browser/renderer_host/input/touch_event_queue.cc:737: const > WebTouchPoint& current_point) const { > Nit: This should be a free function in the anonymous namespace at the top of the > file (below |OutsideApplicationSlopRegion(...)|). > > https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... > content/browser/renderer_host/input/touch_event_queue.cc:745: } else { > Nit: Ditch the else condition, just end with return false. > > https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... > File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): > > https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... > content/browser/renderer_host/input/touch_event_queue_unittest.cc:2259: > TEST_F(TouchEventQueueTest, TouchMoveTouchesState) { > Could you also add a test (with just one pointer) to verify that the pointer > state doesn't become stationary when properties beside position change? Thank you. I will update soon.
In current patch I added one method for changing both RotationAngle and force. May be i will split it to two. your comments please.
https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:737: const WebTouchPoint& current_point) const { On 2015/01/28 16:18:38, jdduke wrote: > Nit: This should be a free function in the anonymous namespace at the top of the > file (below |OutsideApplicationSlopRegion(...)|). Done. https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:745: } else { On 2015/01/28 16:18:38, jdduke wrote: > Nit: Ditch the else condition, just end with return false. Done. https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/160001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:2259: TEST_F(TouchEventQueueTest, TouchMoveTouchesState) { On 2015/01/28 16:18:38, jdduke wrote: > Could you also add a test (with just one pointer) to verify that the pointer > state doesn't become stationary when properties beside position change? Done.
Almost there, thanks! https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:67: last_point.screenPosition != current_point.screenPosition) { Nit: Let's put the |screenPosition| comparison first (just before position), as it's the most likely to differ. https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:172: void ChangeTouchPointRadius(int index, float radiusX, float radiusY) { radius_x, radius_y. https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:173: touch_event_.ChangePointRadius(index, radiusX, radiusY); Let's instead just modify the touch_event_ pointer properties directly. https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:178: float rotationAngle, rotation_angle https://codereview.chromium.org/788923002/diff/180001/content/common/input/sy... File content/common/input/synthetic_web_input_event_builders.h (right): https://codereview.chromium.org/788923002/diff/180001/content/common/input/sy... content/common/input/synthetic_web_input_event_builders.h:74: void ChangePointRadius(int index, float radiusX, float radiusY); I don't think we need these extra methods, you can simply modify the properties directly (they're public). The other methods are a bit more complicated as they change the pointer count/state.
https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue.cc (right): https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue.cc:67: last_point.screenPosition != current_point.screenPosition) { On 2015/01/29 16:53:19, jdduke wrote: > Nit: Let's put the |screenPosition| comparison first (just before position), as > it's the most likely to differ. Done. https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... File content/browser/renderer_host/input/touch_event_queue_unittest.cc (right): https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:172: void ChangeTouchPointRadius(int index, float radiusX, float radiusY) { On 2015/01/29 16:53:19, jdduke wrote: > radius_x, radius_y. Done. My bad. made some silly mistakes. https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:173: touch_event_.ChangePointRadius(index, radiusX, radiusY); On 2015/01/29 16:53:19, jdduke wrote: > Let's instead just modify the touch_event_ pointer properties directly. Done. https://codereview.chromium.org/788923002/diff/180001/content/browser/rendere... content/browser/renderer_host/input/touch_event_queue_unittest.cc:178: float rotationAngle, On 2015/01/29 16:53:19, jdduke wrote: > rotation_angle Done. https://codereview.chromium.org/788923002/diff/180001/content/common/input/sy... File content/common/input/synthetic_web_input_event_builders.h (right): https://codereview.chromium.org/788923002/diff/180001/content/common/input/sy... content/common/input/synthetic_web_input_event_builders.h:74: void ChangePointRadius(int index, float radiusX, float radiusY); On 2015/01/29 16:53:19, jdduke wrote: > I don't think we need these extra methods, you can simply modify the properties > directly (they're public). The other methods are a bit more complicated as they > change the pointer count/state. Done.
@jdduke: Any comments on latest patch set?
lgtm, thanks for your patience.
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/02/02 19:15:37, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > ios_dbg_simulator on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) > ios_rel_device_ng on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) > ios_rel_device_ninja_ng on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) > mac_chromium_rel_ng on tryserver.chromium.mac > (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) You need to rebase.
The CQ bit was checked by s.singapati@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/788923002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/750a80d9cce15010794afcd80f514e67421e2c01 Cr-Commit-Position: refs/heads/master@{#314315} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
