Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2398343002:
X11: Avoid round-tripping to get the cursor position (Closed)
|
Created:
4 years, 2 months ago by Tom (Use chromium acct) Modified:
4 years, 2 months ago Reviewers:
sadrul CC:
chromium-reviews, tfarina, tdresser+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionX11: Avoid round-tripping to get the cursor position
This is a performance optimization which avoids calling XQueryPointer
when we're dispatching a mouse event. It's become a convenience to
call GetCursorScreenPoint() all over the place in Chrome, even when an
event which has the cursor position is available.
R=sadrul@chromium.org
Committed: https://crrev.com/6753a182d50ded7ddcbb91304bf71c6408c49f52
Cr-Commit-Position: refs/heads/master@{#424645}
Patch Set 1 #Patch Set 2 : Revert install-sysroot thing #
Total comments: 10
Patch Set 3 : Add event stack #Patch Set 4 : Actually add event stack #
Total comments: 6
Patch Set 5 : Use EventSystemLocationFromNative and base::nullopt #
Total comments: 2
Patch Set 6 : Add DCHECK #
Messages
Total messages: 43 (29 generated)
The CQ bit was checked by thomasanderson@google.com to run a CQ dry run
Description was changed from ========== X11: Avoid round-tripping to get the cursor position This is a performance optimization which avoids calling XQueryPointer when we're dispatching a mouse event. It's become a convenience to call GetCursorScreenPoint() all over the place in Chrome, even when an event which has the cursor position is available. ========== to ========== X11: Avoid round-tripping to get the cursor position This is a performance optimization which avoids calling XQueryPointer when we're dispatching a mouse event. It's become a convenience to call GetCursorScreenPoint() all over the place in Chrome, even when an event which has the cursor position is available. R=sadrul@chromium.org ==========
thomasanderson@google.com changed reviewers: + sadrul@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:154: const XIEvent* xi_event = Do we need to check for deviceid or sourceid here? https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:169: } Do we also need to handle XI_Touch{Begin,Update,End}?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:208: delegate_->ProcessXEvent(xevent); This can trigger a nested message loop, which means |dispatching_event_| will not be reset until the nested message loop ends. So if some code tries to get the current cursor location (from a timeout task, for example), then it will get stale information. https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:82: const XEvent* get_dispatching_event() const { return dispatching_event_; } Exposing the raw XEvent feels more icky. I would prefer something like 'bool GetRootCursorLocationFromCurrentEvent(gfx::Point* location)' or something like that. https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:154: const XIEvent* xi_event = On 2016/10/07 00:23:18, Tom Anderson wrote: > Do we need to check for deviceid or sourceid here? Probably check with TouchFactory first (like in https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...) https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:169: } On 2016/10/07 00:23:18, Tom Anderson wrote: > Do we also need to handle XI_Touch{Begin,Update,End}? Let's not. chrome generally expects that touch events do not affect the mouse cursor.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by thomasanderson@google.com 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...
This seems to eliminate most of the round trips caused by GetCursorScreenPoint, however round trips are slipping through during tab-dragging caused by some delayed callback. Not sure if it's worth debugging, but the stack looks like this #1 0x7f7dd681fe59 views::DesktopScreenX11::GetCursorScreenPoint() #2 0x7f7de6e33ed2 TabDragController::GetCursorScreenPoint() #3 0x7f7de6e33e7e TabDragController::OnWidgetBoundsChanged() #4 0x7f7dd67b8215 views::Widget::OnNativeWidgetMove() #5 0x7f7dd682b7aa views::DesktopWindowTreeHostX11::SetBounds() #6 0x7f7dd684443d views::X11DesktopWindowMoveClient::OnMouseMovement() #7 0x7f7dd684640a views::X11WholeScreenMoveLoop::DispatchMouseMovement() #8 0x7f7dd6847e1e _ZN4base8internal13FunctorTraitsIMN5views22X11WholeScreenMoveLoopEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_ #9 0x7f7dd6847d7a _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5views22X11WholeScreenMoveLoopEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_ #10 0x7f7dd6847d02 _ZN4base8internal7InvokerINS0_9BindStateIMN5views22X11WholeScreenMoveLoopEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #11 0x7f7dd6847c4c _ZN4base8internal7InvokerINS0_9BindStateIMN5views22X11WholeScreenMoveLoopEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #12 0x7f7de261cf6b base::internal::RunMixin<>::Run() #13 0x7f7de2652481 base::debug::TaskAnnotator::RunTask() #14 0x7f7de26e24bf base::MessageLoop::RunTask() #15 0x7f7de26e2704 base::MessageLoop::DeferOrRunPendingTask() #16 0x7f7de26e29ce base::MessageLoop::DoWork() #17 0x7f7de26fa2e6 base::MessagePumpGlib::Run() #18 0x7f7de26e2076 base::MessageLoop::RunHandler() #19 0x7f7de2787224 base::RunLoop::Run() #20 0x7f7dd6846d07 views::X11WholeScreenMoveLoop::RunMoveLoop() #21 0x7f7dd6844532 views::X11DesktopWindowMoveClient::RunMoveLoop() #22 0x7f7dd682a2f9 views::DesktopWindowTreeHostX11::RunMoveLoop() #23 0x7f7dd680c5c3 views::DesktopNativeWidgetAura::RunMoveLoop() #24 0x7f7dd67b5fe2 views::Widget::RunMoveLoop() #25 0x7f7de6e32fde TabDragController::RunMoveLoop() #26 0x7f7de6e31332 TabDragController::Drag() #27 0x7f7de6b5a0a1 TabStrip::ContinueDrag() #28 0x7f7de6b4b2cf Tab::OnMouseDragged() #29 0x7f7dd67995d8 views::View::ProcessMouseDragged() #30 0x7f7dd6798ebb views::View::OnMouseEvent() #31 0x7f7dd63ec913 ui::EventHandler::OnEvent() #32 0x7f7dd63e7d90 ui::EventDispatcher::DispatchEvent() #33 0x7f7dd63e767b ui::EventDispatcher::ProcessEvent() #34 0x7f7dd63e7412 ui::EventDispatcherDelegate::DispatchEventToTarget() #35 0x7f7dd63e72bf ui::EventDispatcherDelegate::DispatchEvent() #36 0x7f7dd67b03b3 views::internal::RootView::OnMouseDragged() #37 0x7f7dd67b88e0 views::Widget::OnMouseEvent() #38 0x7f7dd680d081 views::DesktopNativeWidgetAura::OnMouseEvent() #39 0x7f7dd63ec913 ui::EventHandler::OnEvent() #40 0x7f7dd63e7d90 ui::EventDispatcher::DispatchEvent() #41 0x7f7dd63e767b ui::EventDispatcher::ProcessEvent() #42 0x7f7dd63e7412 ui::EventDispatcherDelegate::DispatchEventToTarget() #43 0x7f7dd63e72bf ui::EventDispatcherDelegate::DispatchEvent() #44 0x7f7dd63ed5b1 ui::EventProcessor::OnEventFromSource() #45 0x7f7dd63ee71f ui::EventSource::DeliverEventToProcessor() #46 0x7f7dd63ee337 ui::EventSource::SendEventToProcessor() #47 0x7f7dd682c415 views::DesktopWindowTreeHostX11::DispatchMouseEvent() #48 0x7f7dd682d551 views::DesktopWindowTreeHostX11::DispatchEvent() #49 0x7f7ddfbb7a1f ui::PlatformEventSource::DispatchEvent() #50 0x7f7dcfb7c084 ui::X11EventSourceGlib::ProcessXEvent() #51 0x7f7dcfb689f1 ui::X11EventSource::ExtractCookieDataDispatchEvent() #52 0x7f7dcfb6893a ui::X11EventSource::DispatchXEvents() https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.cc:208: delegate_->ProcessXEvent(xevent); On 2016/10/07 02:12:51, sadrul wrote: > This can trigger a nested message loop, which means |dispatching_event_| will > not be reset until the nested message loop ends. So if some code tries to get > the current cursor location (from a timeout task, for example), then it will get > stale information. done: added a stack of events to handle this. https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... File ui/events/platform/x11/x11_event_source.h (right): https://codereview.chromium.org/2398343002/diff/20001/ui/events/platform/x11/... ui/events/platform/x11/x11_event_source.h:82: const XEvent* get_dispatching_event() const { return dispatching_event_; } On 2016/10/07 02:12:51, sadrul wrote: > Exposing the raw XEvent feels more icky. I would prefer something like 'bool > GetRootCursorLocationFromCurrentEvent(gfx::Point* location)' or something like > that. Done. https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right): https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:154: const XIEvent* xi_event = On 2016/10/07 02:12:51, sadrul wrote: > On 2016/10/07 00:23:18, Tom Anderson wrote: > > Do we need to check for deviceid or sourceid here? > > Probably check with TouchFactory first (like in > https://cs.chromium.org/chromium/src/ui/views/widget/desktop_aura/desktop_win...) Done. https://codereview.chromium.org/2398343002/diff/20001/ui/views/widget/desktop... ui/views/widget/desktop_aura/desktop_screen_x11.cc:169: } On 2016/10/07 02:12:51, sadrul wrote: > On 2016/10/07 00:23:18, Tom Anderson wrote: > > Do we also need to handle XI_Touch{Begin,Update,End}? > > Let's not. chrome generally expects that touch events do not affect the mouse > cursor. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't see any of the changes mentioned in the latest patchset.
Also, have you considered listening for mouse-events in the X11 root window instead?
On 2016/10/09 03:32:43, sadrul wrote: > I don't see any of the changes mentioned in the latest patchset. Oops, I uploaded the old one on accident. Please check the latest one :P On 2016/10/09 03:34:55, sadrul wrote: > Also, have you considered listening for mouse-events in the X11 root window > instead? I gave that a try but didn't get any events. I suppose an alternative for XI2 systems could be to detect raw pointer motion events and cache that. But this seems wasteful waking up the chrome process every time the cursor moves, and can't be good for battery life on laptops.
The CQ bit was checked by thomasanderson@google.com 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.
Some nits. https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:197: XEvent* event = Early return if |dispatching_events_| is empty. https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:231: } Use ui::EventSystemLocationFromNative(), e.g.: bool is_valid_event = false; switch (event->type) { ... } if (is_valid_event) return ui::EventSystemLocationFromNative(event); return base::nullopt; https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:233: return base::Optional<gfx::Point>(); return base::nullopt?
https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:197: XEvent* event = On 2016/10/11 16:25:02, sadrul wrote: > Early return if |dispatching_events_| is empty. Done. https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:231: } On 2016/10/11 16:25:02, sadrul wrote: > Use ui::EventSystemLocationFromNative(), e.g.: > bool is_valid_event = false; > switch (event->type) { > ... > } > if (is_valid_event) > return ui::EventSystemLocationFromNative(event); > return base::nullopt; Done. https://codereview.chromium.org/2398343002/diff/140001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:233: return base::Optional<gfx::Point>(); On 2016/10/11 16:25:02, sadrul wrote: > return base::nullopt? Done.
The CQ bit was checked by thomasanderson@google.com 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.
https://codereview.chromium.org/2398343002/diff/160001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2398343002/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:202: if (event) { Should this be a DCHECK()? This should never be null, right?
https://codereview.chromium.org/2398343002/diff/160001/ui/events/platform/x11... File ui/events/platform/x11/x11_event_source.cc (right): https://codereview.chromium.org/2398343002/diff/160001/ui/events/platform/x11... ui/events/platform/x11/x11_event_source.cc:202: if (event) { On 2016/10/12 00:17:17, sadrul wrote: > Should this be a DCHECK()? This should never be null, right? Done.
The CQ bit was checked by thomasanderson@google.com 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...
lgtm
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 thomasanderson@google.com
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 ========== X11: Avoid round-tripping to get the cursor position This is a performance optimization which avoids calling XQueryPointer when we're dispatching a mouse event. It's become a convenience to call GetCursorScreenPoint() all over the place in Chrome, even when an event which has the cursor position is available. R=sadrul@chromium.org ========== to ========== X11: Avoid round-tripping to get the cursor position This is a performance optimization which avoids calling XQueryPointer when we're dispatching a mouse event. It's become a convenience to call GetCursorScreenPoint() all over the place in Chrome, even when an event which has the cursor position is available. R=sadrul@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== X11: Avoid round-tripping to get the cursor position This is a performance optimization which avoids calling XQueryPointer when we're dispatching a mouse event. It's become a convenience to call GetCursorScreenPoint() all over the place in Chrome, even when an event which has the cursor position is available. R=sadrul@chromium.org ========== to ========== X11: Avoid round-tripping to get the cursor position This is a performance optimization which avoids calling XQueryPointer when we're dispatching a mouse event. It's become a convenience to call GetCursorScreenPoint() all over the place in Chrome, even when an event which has the cursor position is available. R=sadrul@chromium.org Committed: https://crrev.com/6753a182d50ded7ddcbb91304bf71c6408c49f52 Cr-Commit-Position: refs/heads/master@{#424645} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6753a182d50ded7ddcbb91304bf71c6408c49f52 Cr-Commit-Position: refs/heads/master@{#424645} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
