Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2429953002:
Implement compositor thread VSync aligned event queue (Closed)
DescriptionImplement compositor thread VSync aligned event queue
Add compositor thread event queue to |InputHandlerProxy|, queues
|GestureScroll/Pinch| and dispatch on |WillBeginImplFrame()| signal
from |LayerTreeHost|.
Currently behind a feature flag and is disabled.
Future Task:
Coalesce events across start/end boundaries.
Design Doc:
https://docs.google.com/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpHBo/edit?usp=sharing
BUG=625689
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/679c4fba7375c35179c6802970d7b4255ba1b14d
Cr-Commit-Position: refs/heads/master@{#433275}
Patch Set 1 #Patch Set 2 : dtapuska's review: Non-template CompositorThreadEventQueue; Added blink_features.h; UMA only when e… #
Total comments: 29
Patch Set 3 : tdresser's review: Rename UMA entries and nits #
Total comments: 2
Patch Set 4 : tdresser's review: Move CanCoalesce/Coalesce; Test queueing time #
Total comments: 10
Patch Set 5 : dtapuska's review: Use TickClock; Remove compositor_event_queue_enabled_; Nits #
Total comments: 11
Patch Set 6 : dtapuska&enne's review: Call |Now()| once per loop; Chromium style; Remove parameterized test #Messages
Total messages: 102 (85 generated)
Description was changed from ========== Add VSync event queue for compositor thread BUG=625689 ========== to ========== Add VSync event queue for compositor thread BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
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 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_...)
Patchset #1 (id:1) has been deleted
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Add VSync event queue for compositor thread BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Implement compositor thread VSync aligned event queue Add compositor thread event queue to |InputHandlerProxy|, queues |GestureScroll/Pinch| and dispatch on |WillBeginImplFrame()| signal from |LayerTreeHost|. Currently behind a feature flag and is disabled. Future Task: Coalesce events across start/end boundaries. Design Doc: https://docs.google.com/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpH... BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
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:80001) has been deleted
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
dtapuska@ PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
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:100001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chongz@chromium.org changed reviewers: + tdresser@chromium.org
dtapuska@ I've updated the CL as per your comments: 1. Change |CompositorThreadEventQueue| to non-template class; 2. Move feature to 'blink_features.h'; 3. Record UMA only when VSync is enabled. PTAL again, thanks! tdresser@ Can you take a look as well? Thanks!
.pak file looks accidental. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... File ui/events/blink/blink_features.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.cc:12: } newline before }, update comment. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... File ui/events/blink/blink_features.h (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.h:12: // VSync aligned input events support. Let's clarify that this is for compositor gestures only. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.h:14: } inconsistent whitespace for namespace brackets. I think you want a newline before the } https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/compos... File ui/events/blink/compositor_thread_event_queue.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/compos... ui/events/blink/compositor_thread_event_queue.cc:15: if (!queue_.empty() && queue_.back()->CanCoalesceWith(*event)) { This doesn't handle the scroll/pinch/scroll case, does it? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... File ui/events/blink/event_with_callback.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... ui/events/blink/event_with_callback.cc:38: WebGestureEvent* event) { Don't we have coalescing logic elsewhere already? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... ui/events/blink/event_with_callback.cc:139: } // namespace ui newline before } https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:330: bool needsAnimateInput = event_queue_->empty(); needs_animate_input https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:337: // We have to dispatch the event to know whether it's on compositor or not. to know whether the gesture sequence will be handled by the compositor or not. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:346: // the zero value would be dominated by non-continuous events. I'm not convinced that this would actually be dominated by non-continuous events. I agree this is the right thing to do (because the signal will be less noisy this way), but we might want to reword the comment. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:355: "Event.CompositorThreadEventQueue.Continuous.FreshnessTime", Can we use a more explicit name than "FreshnessTime"? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:404: void InputHandlerProxy::DispatchQueuedInputEvent() { DispatchQueuedInputEvents? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.h:240: bool has_ongoing_compositor_scroll_pinch; Missing trailing _
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
tdresser@ I've updated CL as per your comments, PTAL again, thanks! https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... File ui/events/blink/blink_features.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.cc:12: } On 2016/11/01 18:12:43, tdresser wrote: > newline before }, update comment. Done. It was removed by 'cl format'. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... File ui/events/blink/blink_features.h (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.h:12: // VSync aligned input events support. On 2016/11/01 18:12:44, tdresser wrote: > Let's clarify that this is for compositor gestures only. Done. Moved to .cc file according to patterns in: https://cs.chromium.org/chromium/src/content/public/common/content_features.c... https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.h:14: } On 2016/11/01 18:12:44, tdresser wrote: > inconsistent whitespace for namespace brackets. I think you want a newline > before the } Done. It was removed by 'cl format'. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/compos... File ui/events/blink/compositor_thread_event_queue.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/compos... ui/events/blink/compositor_thread_event_queue.cc:15: if (!queue_.empty() && queue_.back()->CanCoalesceWith(*event)) { On 2016/11/01 18:12:44, tdresser wrote: > This doesn't handle the scroll/pinch/scroll case, does it? No it doesn't. I've filed https://crbug.com/661601 for this improvement. Can I add support in the next CL or do you want me to do it now? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... File ui/events/blink/event_with_callback.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... ui/events/blink/event_with_callback.cc:38: WebGestureEvent* event) { On 2016/11/01 18:12:44, tdresser wrote: > Don't we have coalescing logic elsewhere already? Yes I copied from "content/common/input/event_with_latency_info.h/.cc". I thought we can't import "content" from "ui", no? Or do you want me to move the logic here? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... ui/events/blink/event_with_callback.cc:139: } // namespace ui On 2016/11/01 18:12:44, tdresser wrote: > newline before } Done. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:330: bool needsAnimateInput = event_queue_->empty(); On 2016/11/01 18:12:44, tdresser wrote: > needs_animate_input Done. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:337: // We have to dispatch the event to know whether it's on compositor or not. On 2016/11/01 18:12:44, tdresser wrote: > to know whether the gesture sequence will be handled by the compositor or not. Done. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:346: // the zero value would be dominated by non-continuous events. On 2016/11/01 18:12:44, tdresser wrote: > I'm not convinced that this would actually be dominated by non-continuous > events. I agree this is the right thing to do (because the signal will be less > noisy this way), but we might want to reword the comment. Reworded to ``` Report the coalesced count only for continuous events to avoid the noise from non-continuous events. ``` https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:355: "Event.CompositorThreadEventQueue.Continuous.FreshnessTime", On 2016/11/01 18:12:44, tdresser wrote: > Can we use a more explicit name than "FreshnessTime"? Renamed to "HeadQueueingTime" and "TailQueueingTime". https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:404: void InputHandlerProxy::DispatchQueuedInputEvent() { On 2016/11/01 18:12:44, tdresser wrote: > DispatchQueuedInputEvents? Done. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.h:240: bool has_ongoing_compositor_scroll_pinch; On 2016/11/01 18:12:44, tdresser wrote: > Missing trailing _ Done.
Looks like we're missing tests for some histograms. Other than that, this LGTM (but get Dave's review). https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... File ui/events/blink/blink_features.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.cc:12: } On 2016/11/02 21:29:32, chongz wrote: > On 2016/11/01 18:12:43, tdresser wrote: > > newline before }, update comment. > > Done. It was removed by 'cl format'. If git cl format removes the whitespace, then perhaps to make it consistent, we should instead have removed the newline after "namespace features {"? https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/compos... File ui/events/blink/compositor_thread_event_queue.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/compos... ui/events/blink/compositor_thread_event_queue.cc:15: if (!queue_.empty() && queue_.back()->CanCoalesceWith(*event)) { On 2016/11/02 21:29:32, chongz wrote: > On 2016/11/01 18:12:44, tdresser wrote: > > This doesn't handle the scroll/pinch/scroll case, does it? > > No it doesn't. I've filed https://crbug.com/661601 for this improvement. Can I > add support in the next CL or do you want me to do it now? Next CL is fine. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... File ui/events/blink/event_with_callback.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... ui/events/blink/event_with_callback.cc:38: WebGestureEvent* event) { On 2016/11/02 21:29:32, chongz wrote: > On 2016/11/01 18:12:44, tdresser wrote: > > Don't we have coalescing logic elsewhere already? > > Yes I copied from "content/common/input/event_with_latency_info.h/.cc". > > I thought we can't import "content" from "ui", no? Or do you want me to move the > logic here? Maybe move it to https://cs.chromium.org/chromium/src/ui/events/blink/blink_event_util.h?sq=pa... ? https://codereview.chromium.org/2429953002/diff/160001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/160001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:233: bool IsGestureScollOrPinch(WebInputEvent::Type type) { These should probably go in the event utils file.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Patchset #4 (id:180001) has been deleted
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...
@dtapuska PTAL again, thanks! > Looks like we're missing tests for some histograms. Added tests for queueing time, but have to do some hack and I'm not sure if there is a better way. > Other than that, this LGTM (but get Dave's review). https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... File ui/events/blink/blink_features.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/blink_... ui/events/blink/blink_features.cc:12: } On 2016/11/04 16:51:03, tdresser wrote: > On 2016/11/02 21:29:32, chongz wrote: > > On 2016/11/01 18:12:43, tdresser wrote: > > > newline before }, update comment. > > > > Done. It was removed by 'cl format'. > > If git cl format removes the whitespace, then perhaps to make it consistent, we > should instead have removed the newline after > > "namespace features {"? Just checked other files in this directory, and it seems to be bug of cl-format and we should have newline before&after namespace. https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... File ui/events/blink/event_with_callback.cc (right): https://codereview.chromium.org/2429953002/diff/140001/ui/events/blink/event_... ui/events/blink/event_with_callback.cc:38: WebGestureEvent* event) { On 2016/11/04 16:51:03, tdresser wrote: > On 2016/11/02 21:29:32, chongz wrote: > > On 2016/11/01 18:12:44, tdresser wrote: > > > Don't we have coalescing logic elsewhere already? > > > > Yes I copied from "content/common/input/event_with_latency_info.h/.cc". > > > > I thought we can't import "content" from "ui", no? Or do you want me to move > the > > logic here? > > Maybe move it to > https://cs.chromium.org/chromium/src/ui/events/blink/blink_event_util.h?sq=pa... > ? Done. https://codereview.chromium.org/2429953002/diff/160001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/160001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:233: bool IsGestureScollOrPinch(WebInputEvent::Type type) { On 2016/11/04 16:51:03, tdresser wrote: > These should probably go in the event utils file. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/compos... File ui/events/blink/compositor_thread_event_queue.h (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/compos... ui/events/blink/compositor_thread_event_queue.h:38: typedef std::deque<std::unique_ptr<EventWithCallback>> EventQueue; using is prefered now instead of typedef. https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/event_... File ui/events/blink/event_with_callback.h (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/event_... ui/events/blink/event_with_callback.h:61: static std::unique_ptr<base::TimeTicks> testing_timestamp_now_; This is kind of clunky. I'd prefer and examples I have seen in unit tests is to pass the TickClock in.. And have the unit test implement a TickClock. There are numerous examples; https://cs.chromium.org/chromium/src/net/socket/tcp_socket_posix.h?q=TickCloc... You'll need to probably somehow be able to set the tick clock on your input handler proxy but that probably is alright. https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:267: event_queue_ = base::MakeUnique<CompositorThreadEventQueue>(); no scope brackets https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:362: case blink::WebGestureEvent::GestureScrollUpdate: Can this be combined with the above cases? https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.h:242: bool compositor_event_queue_enabled_; Can we use the existence of the event_queue_ as knowledge we are supporting it? ie; I don't like states that two values imply each other. For example compositor_event_queue_enabled_ could be true but event_queue_ nullptr
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #5 (id:220001) 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...
dtapuska@ I've updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/compos... File ui/events/blink/compositor_thread_event_queue.h (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/compos... ui/events/blink/compositor_thread_event_queue.h:38: typedef std::deque<std::unique_ptr<EventWithCallback>> EventQueue; On 2016/11/09 21:28:05, dtapuska wrote: > using is prefered now instead of typedef. Done. https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/event_... File ui/events/blink/event_with_callback.h (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/event_... ui/events/blink/event_with_callback.h:61: static std::unique_ptr<base::TimeTicks> testing_timestamp_now_; On 2016/11/09 21:28:05, dtapuska wrote: > This is kind of clunky. I'd prefer and examples I have seen in unit tests is to > pass the TickClock in.. > > And have the unit test implement a TickClock. There are numerous examples; > https://cs.chromium.org/chromium/src/net/socket/tcp_socket_posix.h?q=TickCloc... > > You'll need to probably somehow be able to set the tick clock on your input > handler proxy but that probably is alright. Done. https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:267: event_queue_ = base::MakeUnique<CompositorThreadEventQueue>(); On 2016/11/09 21:28:05, dtapuska wrote: > no scope brackets Done. https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:362: case blink::WebGestureEvent::GestureScrollUpdate: On 2016/11/09 21:28:05, dtapuska wrote: > Can this be combined with the above cases? Done. https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.h (right): https://codereview.chromium.org/2429953002/diff/200001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.h:242: bool compositor_event_queue_enabled_; On 2016/11/09 21:28:05, dtapuska wrote: > Can we use the existence of the event_queue_ as knowledge we are supporting it? > ie; I don't like states that two values imply each other. For example > compositor_event_queue_enabled_ could be true but event_queue_ nullptr Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dtapuska@chromium.org changed reviewers: + enne@chromium.org
enne@ do you have any feedback on this? https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:296: !IsGestureScollOrPinch(event_with_callback->event().type)) { This might need a comment. I look at this line and I see that other input can race ahead of gesture input; perhaps we should indicate that we believe this is ok to do.
On 2016/11/14 18:18:45, dtapuska wrote: > enne@ do you have any feedback on this? > > https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... > File ui/events/blink/input_handler_proxy.cc (right): > > https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... > ui/events/blink/input_handler_proxy.cc:296: > !IsGestureScollOrPinch(event_with_callback->event().type)) { > This might need a comment. I look at this line and I see that other input can > race ahead of gesture input; perhaps we should indicate that we believe this is > ok to do. lgtm % a comment
lgtm in general. This all seems quite reasonable to me with a few nits. https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/event_... File ui/events/blink/event_with_callback.h (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/event_... ui/events/blink/event_with_callback.h:32: const LatencyInfo latencyInfo() const { return latency_; } Should this interface be in Chromium style, e.g. latency_info or GetLatencyInfo? https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:321: base::TimeTicks now = tick_clock_->NowTicks(); For what it's worth, I know brianderson has mentioned to me that he's seen calling Now() be expensive when called repeatedly. This is maybe premature optimization, but it might be worth passing this in as a parameter since events are often dispatched in a loop with this queue. https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy_unittest.cc:496: float deltaYOrScale = 0, style nit: more blink style here and elsewhere https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy_unittest.cc:3107: INSTANTIATE_TEST_CASE_P(InputHandlerProxyEventQueueTests, Why is this parameterized? It looks like all cases of InputHandlerProxyEventQueueTest do nothing when the feature is turned off?
https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:321: base::TimeTicks now = tick_clock_->NowTicks(); On 2016/11/14 23:59:46, enne wrote: > For what it's worth, I know brianderson has mentioned to me that he's seen > calling Now() be expensive when called repeatedly. This is maybe premature > optimization, but it might be worth passing this in as a parameter since events > are often dispatched in a loop with this queue. I think this is worth doing.
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #6 (id:300001) 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...
chongz@chromium.org changed reviewers: + isherman@chromium.org
isherman@ PTAL at "tools/metrics/histograms/histograms.xml", thanks! https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/event_... File ui/events/blink/event_with_callback.h (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/event_... ui/events/blink/event_with_callback.h:32: const LatencyInfo latencyInfo() const { return latency_; } On 2016/11/14 23:59:46, enne wrote: > Should this interface be in Chromium style, e.g. latency_info or GetLatencyInfo? Done. https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy.cc (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:296: !IsGestureScollOrPinch(event_with_callback->event().type)) { On 2016/11/14 18:18:45, dtapuska wrote: > This might need a comment. I look at this line and I see that other input can > race ahead of gesture input; perhaps we should indicate that we believe this is > ok to do. Done. https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy.cc:321: base::TimeTicks now = tick_clock_->NowTicks(); On 2016/11/15 00:09:22, tdresser wrote: > On 2016/11/14 23:59:46, enne wrote: > > For what it's worth, I know brianderson has mentioned to me that he's seen > > calling Now() be expensive when called repeatedly. This is maybe premature > > optimization, but it might be worth passing this in as a parameter since > events > > are often dispatched in a loop with this queue. > > I think this is worth doing. Done. https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... File ui/events/blink/input_handler_proxy_unittest.cc (right): https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy_unittest.cc:496: float deltaYOrScale = 0, On 2016/11/14 23:59:46, enne wrote: > style nit: more blink style here and elsewhere Done. https://codereview.chromium.org/2429953002/diff/240001/ui/events/blink/input_... ui/events/blink/input_handler_proxy_unittest.cc:3107: INSTANTIATE_TEST_CASE_P(InputHandlerProxyEventQueueTests, On 2016/11/14 23:59:46, enne wrote: > Why is this parameterized? It looks like all cases of > InputHandlerProxyEventQueueTest do nothing when the feature is turned off? Removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
metrics lgtm, thanks
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, dtapuska@chromium.org, enne@chromium.org Link to the patchset: https://codereview.chromium.org/2429953002/#ps320001 (title: "dtapuska&enne's review: Call |Now()| once per loop; Chromium style; Remove parameterized test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement compositor thread VSync aligned event queue Add compositor thread event queue to |InputHandlerProxy|, queues |GestureScroll/Pinch| and dispatch on |WillBeginImplFrame()| signal from |LayerTreeHost|. Currently behind a feature flag and is disabled. Future Task: Coalesce events across start/end boundaries. Design Doc: https://docs.google.com/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpH... BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Implement compositor thread VSync aligned event queue Add compositor thread event queue to |InputHandlerProxy|, queues |GestureScroll/Pinch| and dispatch on |WillBeginImplFrame()| signal from |LayerTreeHost|. Currently behind a feature flag and is disabled. Future Task: Coalesce events across start/end boundaries. Design Doc: https://docs.google.com/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpH... BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #6 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Implement compositor thread VSync aligned event queue Add compositor thread event queue to |InputHandlerProxy|, queues |GestureScroll/Pinch| and dispatch on |WillBeginImplFrame()| signal from |LayerTreeHost|. Currently behind a feature flag and is disabled. Future Task: Coalesce events across start/end boundaries. Design Doc: https://docs.google.com/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpH... BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Implement compositor thread VSync aligned event queue Add compositor thread event queue to |InputHandlerProxy|, queues |GestureScroll/Pinch| and dispatch on |WillBeginImplFrame()| signal from |LayerTreeHost|. Currently behind a feature flag and is disabled. Future Task: Coalesce events across start/end boundaries. Design Doc: https://docs.google.com/document/d/1SUPPJkqxEYMRso3Jw1wZnqD3g1dmif0ic-x5aQkpH... BUG=625689 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/679c4fba7375c35179c6802970d7b4255ba1b14d Cr-Commit-Position: refs/heads/master@{#433275} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/679c4fba7375c35179c6802970d7b4255ba1b14d Cr-Commit-Position: refs/heads/master@{#433275} |
