Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2677903003:
[VSync Queue] Support multiple in-flight events in |GestureEventQueue| (Closed)
Description[VSync Queue] Support multiple in-flight events in |GestureEventQueue|
Currently |GestureEventQueue| is holding on events until it receives
ack for the last one.
This CL adds support for multiple in-flight events so
|GestureEventQueue| can forward events immediately and have them
coalesced in compositor event queue.
This feature is behind a flag and disabled by default.
BUG=625689
Review-Url: https://codereview.chromium.org/2677903003
Cr-Commit-Position: refs/heads/master@{#449984}
Committed: https://chromium.googlesource.com/chromium/src/+/92f5f351ace34c217f4a2adf3dd717fc30df2fac
Patch Set 1 #
Total comments: 17
Patch Set 2 : tdresser's review #
Total comments: 2
Patch Set 3 : Add switch when searching events to ack #
Messages
Total messages: 29 (22 generated)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Support multiple inflight events BUG=625689 ========== to ========== [VSync Queue] Support multiple in-flight events in |GestureEventQueue| Currently |GestureEventQueue| is holding on events until it receives ack for the last one. This CL adds support for multiple in-flight events so |GestureEventQueue| can forward events immediately and have them coalesced in compositor event queue. This feature is behind a flag and disabled by default. BUG=625689 ==========
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 chongz@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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chongz@chromium.org changed reviewers: + tdresser@chromium.org
tdresser@ PTAL, thanks! https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:167: if (OnScrollBegin(gesture_event)) I think we don't need this anymore after scroll latching, is that correct? https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:380: return coalesced_gesture_events_.size(); Currently not used if |allow_multiple_inflight_events_ == true|, but should be reasonable to have.
https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:167: if (OnScrollBegin(gesture_event)) On 2017/02/07 15:31:07, chongz wrote: > I think we don't need this anymore after scroll latching, is that correct? Yeah, that appears to be the case. We'll clean this up once scroll latching has landed and stuck. https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:207: // Assuming events of the same type come back in a FIFO order. come back -> are acked https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:211: for (size_t i = 0; i < coalesced_gesture_events_.size(); ++i) { We should only ever need to look at the two first events in the queue, shouldn't we? https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:239: // Events should have been forwarded already. I'd get rid of the "should". https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:380: return coalesced_gesture_events_.size(); On 2017/02/07 15:31:07, chongz wrote: > Currently not used if |allow_multiple_inflight_events_ == true|, but should be > reasonable to have. Maybe add a DCHECK and a comment that this is currently never called in this case? https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.h:175: // events and will forward events immediately. (instead of waiting for Extremely nitty nit: Generally you'd leave out the period before the . https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:1199: TEST_F(GestureEventQueueWithCompositorEventQueueTest, MultipleGestureInFlight) { MultipleGesture<s>InFlight https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:1215: // from a point that is not the origin should still give us the right scroll. We're never checking the scroll position are we? This comment implies that we are.
The CQ bit was checked by chongz@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 checked by chongz@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...
Patchset #2 (id:40001) has been deleted
tdresser@ I've updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:207: // Assuming events of the same type come back in a FIFO order. On 2017/02/07 15:51:50, tdresser wrote: > come back -> are acked Done. https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:211: for (size_t i = 0; i < coalesced_gesture_events_.size(); ++i) { On 2017/02/07 15:51:50, tdresser wrote: > We should only ever need to look at the two first events in the queue, shouldn't > we? Sorry for the confusion, I've added more comments. Actually with the compositor event queue we will never ack GestureScroll and GesturePinch in reversed order since we are storing/acking the original events. However it should be possible that GestureScroll/Pinch was queued but other gesture events skipped the queue since we are not coalescing them, or is it not possible in practice? See https://cs.chromium.org/chromium/src/ui/events/blink/input_handler_proxy.cc?q... https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:239: // Events should have been forwarded already. On 2017/02/07 15:51:50, tdresser wrote: > I'd get rid of the "should". Done. https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:380: return coalesced_gesture_events_.size(); On 2017/02/07 15:51:50, tdresser wrote: > On 2017/02/07 15:31:07, chongz wrote: > > Currently not used if |allow_multiple_inflight_events_ == true|, but should be > > reasonable to have. > > Maybe add a DCHECK and a comment that this is currently never called in this > case? Done. https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.h (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.h:175: // events and will forward events immediately. (instead of waiting for On 2017/02/07 15:51:50, tdresser wrote: > Extremely nitty nit: Generally you'd leave out the period before the . Done. https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue_unittest.cc (right): https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:1199: TEST_F(GestureEventQueueWithCompositorEventQueueTest, MultipleGestureInFlight) { On 2017/02/07 15:51:50, tdresser wrote: > MultipleGesture<s>InFlight Done. https://codereview.chromium.org/2677903003/diff/20001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue_unittest.cc:1215: // from a point that is not the origin should still give us the right scroll. On 2017/02/07 15:51:50, tdresser wrote: > We're never checking the scroll position are we? This comment implies that we > are. Sorry for the confusion, this comment was copied from other tests and shouldn't be related to this test. I've removed and updated other comments as well. Anyway I think we are never checking scroll position, and this comment should just be telling that we can coalesce GesturePinch regardless it's anchor.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with nit. https://codereview.chromium.org/2677903003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/2677903003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:221: } Thanks for clarifying this. Despite the fact that it's a bit more code, I'd be tempted to split out based on whether or not |allow_multiple_inflight_events_ == true|, and then have one comment per case. Right now, it isn't clear which of 1 and 2 apply in each situation, and it's a bit harder to follow because it's more generic than required in case #1.
https://codereview.chromium.org/2677903003/diff/60001/content/browser/rendere... File content/browser/renderer_host/input/gesture_event_queue.cc (right): https://codereview.chromium.org/2677903003/diff/60001/content/browser/rendere... content/browser/renderer_host/input/gesture_event_queue.cc:221: } On 2017/02/08 13:42:10, tdresser wrote: > Thanks for clarifying this. Despite the fact that it's a bit more code, I'd be > tempted to split out based on whether or not |allow_multiple_inflight_events_ == > true|, and then have one comment per case. > > Right now, it isn't clear which of 1 and 2 apply in each situation, and it's a > bit harder to follow because it's more generic than required in case #1. Done.
The CQ bit was checked by chongz@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/2677903003/#ps80001 (title: "Add switch when searching events to ack")
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": 80001, "attempt_start_ts": 1487001970656200,
"parent_rev": "30ab8326aac843793024d240225a2ae273477876", "commit_rev":
"92f5f351ace34c217f4a2adf3dd717fc30df2fac"}
Message was sent while issue was closed.
Description was changed from ========== [VSync Queue] Support multiple in-flight events in |GestureEventQueue| Currently |GestureEventQueue| is holding on events until it receives ack for the last one. This CL adds support for multiple in-flight events so |GestureEventQueue| can forward events immediately and have them coalesced in compositor event queue. This feature is behind a flag and disabled by default. BUG=625689 ========== to ========== [VSync Queue] Support multiple in-flight events in |GestureEventQueue| Currently |GestureEventQueue| is holding on events until it receives ack for the last one. This CL adds support for multiple in-flight events so |GestureEventQueue| can forward events immediately and have them coalesced in compositor event queue. This feature is behind a flag and disabled by default. BUG=625689 Review-Url: https://codereview.chromium.org/2677903003 Cr-Commit-Position: refs/heads/master@{#449984} Committed: https://chromium.googlesource.com/chromium/src/+/92f5f351ace34c217f4a2adf3dd7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/92f5f351ace34c217f4a2adf3dd7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
