Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2789203006:
Routes touch ACK events to the correct GestureProvider (Closed)
|
Created:
3 years, 8 months ago by malaykeshav Modified:
3 years, 8 months ago Reviewers:
tdresser CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRoutes touch ACK events to the correct GestureProvider
Currently a call to |GestureRecognizerImpl::TransferEventsTo()| can lead
to an incorrect state for touch events. Specifically, the event ACKS are
getting routed to a different reference of GestureProvider from the one
where the original events were routed to.
This patch stores a mapping of every event that was dispatched along
with its corresponding GestureProvider. This allows the event ACKS to be
routed correctly despite a call made to |TransferEventsTo()|.
BUG=698843
COMPONENT=Gesture Recognizer, Touch Gestures, Drag and Drop
Review-Url: https://codereview.chromium.org/2789203006
Cr-Commit-Position: refs/heads/master@{#462925}
Committed: https://chromium.googlesource.com/chromium/src/+/dc60e34a7fa422c46c7a6abe6b17e1d2b858b5b6
Patch Set 1 #Patch Set 2 : Fixed Tests #
Total comments: 2
Patch Set 3 : nit #Patch Set 4 : Added Bug specific unit test #
Total comments: 11
Patch Set 5 : Resolving comments #Patch Set 6 : nits #
Messages
Total messages: 43 (35 generated)
Patchset #1 (id:1) has been deleted
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
malaykeshav@chromium.org changed reviewers: + tdresser@chromium.org
PTAL
Tests? https://codereview.chromium.org/2789203006/diff/40001/ui/events/gestures/gest... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/2789203006/diff/40001/ui/events/gestures/gest... ui/events/gestures/gesture_recognizer_impl.cc:285: // consumer associted with it. associted -> associated
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL Added tests for the bug https://codereview.chromium.org/2789203006/diff/40001/ui/events/gestures/gest... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/2789203006/diff/40001/ui/events/gestures/gest... ui/events/gestures/gesture_recognizer_impl.cc:285: // consumer associted with it. On 2017/04/05 at 14:23:39, tdresser wrote: > associted -> associated Done
Thanks, looks pretty good - I'd like to give it one more quick skim after you've addressed the feedback. https://codereview.chromium.org/2789203006/diff/100001/ui/aura/gestures/gestu... File ui/aura/gestures/gesture_recognizer_unittest.cc (right): https://codereview.chromium.org/2789203006/diff/100001/ui/aura/gestures/gestu... ui/aura/gestures/gesture_recognizer_unittest.cc:4727: DispatchEventUsingWindowDispatcher(&move2); Does the test need this second move? For that matter, does it need the first move? https://codereview.chromium.org/2789203006/diff/100001/ui/aura/gestures/gestu... ui/aura/gestures/gesture_recognizer_unittest.cc:4743: // ACK for events that were dispatched before the trasnfer should go to the trasnfer -> transfer https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.cc:276: event_to_gesture_provider_[event->unique_event_id()] = gesture_provider; Can we put this in SetupTargets? https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.cc:290: event_to_gesture_provider_.erase(unique_event_id); This is performing three lookups into this map. How about something along the lines of: auto event_to_gesture_provider_iterator = event_to_gesture_provider_.find(unique_event_id) if (event_to_gesture_provider_iterator != event_to_gesture_provider_.end()) { gesture_provider = *event_to_gesture_provider_iterator; event_to_gesture_provider_.erase(event_to_gesture_provider_iterator); } This only does a single index into the map. https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.h:92: // provider. This avoids any invalid reference while routing ACK for events ACK -> ACKs
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2789203006/diff/100001/ui/aura/gestures/gestu... File ui/aura/gestures/gesture_recognizer_unittest.cc (right): https://codereview.chromium.org/2789203006/diff/100001/ui/aura/gestures/gestu... ui/aura/gestures/gesture_recognizer_unittest.cc:4727: DispatchEventUsingWindowDispatcher(&move2); On 2017/04/06 at 20:59:52, tdresser wrote: > Does the test need this second move? > > For that matter, does it need the first move? Removed. https://codereview.chromium.org/2789203006/diff/100001/ui/aura/gestures/gestu... ui/aura/gestures/gesture_recognizer_unittest.cc:4743: // ACK for events that were dispatched before the trasnfer should go to the On 2017/04/06 at 20:59:52, tdresser wrote: > trasnfer -> transfer Done https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... File ui/events/gestures/gesture_recognizer_impl.cc (right): https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.cc:276: event_to_gesture_provider_[event->unique_event_id()] = gesture_provider; On 2017/04/06 at 20:59:52, tdresser wrote: > Can we put this in SetupTargets? Done https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.cc:290: event_to_gesture_provider_.erase(unique_event_id); On 2017/04/06 at 20:59:52, tdresser wrote: > This is performing three lookups into this map. > > How about something along the lines of: > > auto event_to_gesture_provider_iterator = event_to_gesture_provider_.find(unique_event_id) > if (event_to_gesture_provider_iterator != event_to_gesture_provider_.end()) { > gesture_provider = *event_to_gesture_provider_iterator; > event_to_gesture_provider_.erase(event_to_gesture_provider_iterator); > } > > This only does a single index into the map. Done https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.h:92: // provider. This avoids any invalid reference while routing ACK for events On 2017/04/06 at 20:59:52, tdresser wrote: > ACK -> ACKs Doesnt each event have just 1 ACK?
LGTM https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... File ui/events/gestures/gesture_recognizer_impl.h (right): https://codereview.chromium.org/2789203006/diff/100001/ui/events/gestures/ges... ui/events/gestures/gesture_recognizer_impl.h:92: // provider. This avoids any invalid reference while routing ACK for events On 2017/04/07 01:07:59, malaykeshav wrote: > On 2017/04/06 at 20:59:52, tdresser wrote: > > ACK -> ACKs > > Doesnt each event have just 1 ACK? Yes, but you're referring to "events" and events have ACKs. Any of: "while routing an event's ACK" or "while routing ACKs for events" or even just: "while routing an ACK" would be fine.
The CQ bit was checked by malaykeshav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by malaykeshav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2789203006/#ps140001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491587862608650,
"parent_rev": "13745d8f5a636ce8cf5d7eecb09b59cb5be2b0f9", "commit_rev":
"dc60e34a7fa422c46c7a6abe6b17e1d2b858b5b6"}
Message was sent while issue was closed.
Description was changed from ========== Routes touch ACK events to the correct GestureProvider Currently a call to |GestureRecognizerImpl::TransferEventsTo()| can lead to an incorrect state for touch events. Specifically, the event ACKS are getting routed to a different reference of GestureProvider from the one where the original events were routed to. This patch stores a mapping of every event that was dispatched along with its corresponding GestureProvider. This allows the event ACKS to be routed correctly despite a call made to |TransferEventsTo()|. BUG=698843 COMPONENT=Gesture Recognizer, Touch Gestures, Drag and Drop ========== to ========== Routes touch ACK events to the correct GestureProvider Currently a call to |GestureRecognizerImpl::TransferEventsTo()| can lead to an incorrect state for touch events. Specifically, the event ACKS are getting routed to a different reference of GestureProvider from the one where the original events were routed to. This patch stores a mapping of every event that was dispatched along with its corresponding GestureProvider. This allows the event ACKS to be routed correctly despite a call made to |TransferEventsTo()|. BUG=698843 COMPONENT=Gesture Recognizer, Touch Gestures, Drag and Drop Review-Url: https://codereview.chromium.org/2789203006 Cr-Commit-Position: refs/heads/master@{#462925} Committed: https://chromium.googlesource.com/chromium/src/+/dc60e34a7fa422c46c7a6abe6b17... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/dc60e34a7fa422c46c7a6abe6b17... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
