Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2884463002:
Make event-targeting asynchronous in window server. (Closed)
DescriptionMake event-targeting asynchronous in window server.
Change hit-testing in EventTargeter to be asynchronous to accommodate
hit-testing cases that cannot be solved by HittestComponent if the flag
"enable-async-event-targeting" is turned on.
Next steps:
1. Explore hit-test optimization/caching.
2. Once HittestComponent (gklassen@) is implemented, update event
targeting to talk to HittestComponent synchronously for most cases and
talk to blink for "hard" cases that cannot be solved by HittestComponent.
BUG=710016
TEST=service_unittest
Review-Url: https://codereview.chromium.org/2884463002
Cr-Commit-Position: refs/heads/master@{#477783}
Committed: https://chromium.googlesource.com/chromium/src/+/4cd9767ab7c89145bbec6b343ae701c83bf42e9d
Patch Set 1 #
Total comments: 4
Patch Set 2 : WeakPtrFactory; const &; etc #
Total comments: 11
Patch Set 3 : sky@ comments #Patch Set 4 : hit-test-in-flight #Patch Set 5 : std::move #
Total comments: 8
Patch Set 6 : rebase and use EventTargeter #
Total comments: 6
Patch Set 7 : cache; by value #
Total comments: 20
Patch Set 8 : comments #
Total comments: 14
Patch Set 9 : rebase and comments #
Total comments: 8
Patch Set 10 : comments #Patch Set 11 : rebase after https://codereview.chromium.org/2925913002/ #Patch Set 12 : rebase #
Total comments: 14
Patch Set 13 : comments #40 #
Total comments: 8
Patch Set 14 : comments #
Messages
Total messages: 50 (16 generated)
The CQ bit was checked by riajiang@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 ========== Make event-targeting asynchronous in window server. Change hit-testing in EventDispatcher to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=mus_ws_unittests ========== to ========== Make event-targeting asynchronous in window server. Change hit-testing in EventDispatcher to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=mus_ws_unittests ==========
riajiang@chromium.org changed reviewers: + sadrul@chromium.org, sky@chromium.org
Hi, PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I started looking at this and have some questions about how the HittestComponent will work. There are cases where we might ask for hit test component multiple times in succession. Would it be better to cache results from HittestComponent and only ask after a new frame is posted, a delay or the windows change in some way? https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... services/ui/ws/event_dispatcher.cc:636: base::Unretained(this), location, std::move(callback))); What guarantees this is valid by the time the callback runs? https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... services/ui/ws/event_dispatcher.h:176: HittestRequest(gfx::Point location, HittestCallback hittest_callback); const gfx::Point&
On 2017/05/14 15:59:06, sky wrote: > I started looking at this and have some questions about how the HittestComponent > will work. > > There are cases where we might ask for hit test component multiple times in > succession. Would it be better to cache results from HittestComponent and only > ask after a new frame is posted, a delay or the windows change in some way? We do plan on adding some caching mechanism, for example, viz would respond with some cached state (details still to be determined) that the window-server can use to target synchronously until the next begin-frame. I just shared a doc with you that has some more details. > > https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... > File services/ui/ws/event_dispatcher.cc (right): > > https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... > services/ui/ws/event_dispatcher.cc:636: base::Unretained(this), location, > std::move(callback))); > What guarantees this is valid by the time the callback runs? > > https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... > File services/ui/ws/event_dispatcher.h (right): > > https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... > services/ui/ws/event_dispatcher.h:176: HittestRequest(gfx::Point location, > HittestCallback hittest_callback); > const gfx::Point&
https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... services/ui/ws/event_dispatcher.cc:636: base::Unretained(this), location, std::move(callback))); On 2017/05/14 15:59:05, sky wrote: > What guarantees this is valid by the time the callback runs? As discussed, changed to use WeakPtrFactory. https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/1/services/ui/ws/event_dispat... services/ui/ws/event_dispatcher.h:176: HittestRequest(gfx::Point location, HittestCallback hittest_callback); On 2017/05/14 15:59:05, sky wrote: > const gfx::Point& Done.
https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:128: void UpdateNonClientAreaForCurrentWindow(const base::Closure& callback); Having any of these functions take callbacks makes the code hard to reason about. In fact I'm struggling to know if there aren't any unwanted side effects. For processing that needs to happen after the fact how about adding functions to the delegate interface that are called at the right time? The code in WindowServer that runs after updating the native cursor would move to WindowManagerState as WindowManagerState is the EventDispatcherDelegate. https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:149: bool HittestInFlight(); IsHitTestInFlight() and const. Also, use HitTest everywhere (we use HitTest in ui/base/hit_test for one). https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... services/ui/ws/window_manager_state.cc:293: if (in_flight_event_details_ || event_dispatcher_.HittestInFlight()) { The code you have here will only work if EventDispatchDelegate calls to its delegate when the queue is empty so that WMS knows to start processing the next event. https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_s... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_s... services/ui/ws/window_server.cc:604: weak_ptr_factory_.GetWeakPtr(), display_root)); In this code you shouldn't need a weakptrfactory as WindowServer owns everything and will outlive the eventdispatcher. https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_s... services/ui/ws/window_server.cc:624: void WindowServer::OnCursorUpdated(WindowManagerDisplayRoot* display_root) { Style guide says position of declaration and defintion should match.
https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:128: void UpdateNonClientAreaForCurrentWindow(const base::Closure& callback); On 2017/05/15 21:20:23, sky wrote: > Having any of these functions take callbacks makes the code hard to reason > about. In fact I'm struggling to know if there aren't any unwanted side effects. > For processing that needs to happen after the fact how about adding functions to > the delegate interface that are called at the right time? The code in > WindowServer that runs after updating the native cursor would move to > WindowManagerState as WindowManagerState is the EventDispatcherDelegate. Makes sense! With erg@'s CL https://codereview.chromium.org/2857963003, I think it would be sufficient to use the existing WindowManagerState::UpdateNativeCursorFromDispatcher() after updating the native cursor? https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:149: bool HittestInFlight(); On 2017/05/15 21:20:23, sky wrote: > IsHitTestInFlight() and const. Also, use HitTest everywhere (we use HitTest in > ui/base/hit_test for one). Done. https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... services/ui/ws/window_manager_state.cc:293: if (in_flight_event_details_ || event_dispatcher_.HittestInFlight()) { On 2017/05/15 21:20:23, sky wrote: > The code you have here will only work if EventDispatchDelegate calls to its > delegate when the queue is empty so that WMS knows to start processing the next > event. Actually I was looking at |in_flight_event_details_| (https://cs.chromium.org/chromium/src/services/ui/ws/window_manager_state.h?ty...) again and was wondering if it's true that it would be non-null until after we've successfully dispatched the event to the client? (cuz it seems like we only call ProcessNextEventFromQueue() when we are done processing+dispatching the event) If that is true, then it means if there's a hit-test in flight in EventDispatcher, the |in_flight_event_details_| would be non-null so it would already go inside this if-block without checking IsHitTestInFlight()? https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_s... File services/ui/ws/window_server.cc (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_s... services/ui/ws/window_server.cc:604: weak_ptr_factory_.GetWeakPtr(), display_root)); On 2017/05/15 21:20:23, sky wrote: > In this code you shouldn't need a weakptrfactory as WindowServer owns everything > and will outlive the eventdispatcher. Ah I see. And deleted this since not passing a callback anymore.
https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... services/ui/ws/window_manager_state.cc:293: if (in_flight_event_details_ || event_dispatcher_.HittestInFlight()) { On 2017/05/17 02:01:59, riajiang wrote: > On 2017/05/15 21:20:23, sky wrote: > > The code you have here will only work if EventDispatchDelegate calls to its > > delegate when the queue is empty so that WMS knows to start processing the > next > > event. > > Actually I was looking at |in_flight_event_details_| > (https://cs.chromium.org/chromium/src/services/ui/ws/window_manager_state.h?ty...) > again and was wondering if it's true that it would be non-null until after we've > successfully dispatched the event to the client? (cuz it seems like we only call > ProcessNextEventFromQueue() when we are done processing+dispatching the event) > If that is true, then it means if there's a hit-test in flight in > EventDispatcher, the |in_flight_event_details_| would be non-null so it would > already go inside this if-block without checking IsHitTestInFlight()? in_flight_event_details_ is only created from callbacks from EventDispatcher (DispatchInputEventToWindow for one). So, if calling to EventDispatcher doesn't immediately call to DispatchInputEventToWindow() in_flight_event_details_ won't be set to a non-null value.
https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... File services/ui/ws/window_manager_state.cc (right): https://codereview.chromium.org/2884463002/diff/20001/services/ui/ws/window_m... services/ui/ws/window_manager_state.cc:293: if (in_flight_event_details_ || event_dispatcher_.HittestInFlight()) { On 2017/05/17 18:14:43, sky wrote: > On 2017/05/17 02:01:59, riajiang wrote: > > On 2017/05/15 21:20:23, sky wrote: > > > The code you have here will only work if EventDispatchDelegate calls to its > > > delegate when the queue is empty so that WMS knows to start processing the > > next > > > event. > > > > Actually I was looking at |in_flight_event_details_| > > > (https://cs.chromium.org/chromium/src/services/ui/ws/window_manager_state.h?ty...) > > again and was wondering if it's true that it would be non-null until after > we've > > successfully dispatched the event to the client? (cuz it seems like we only > call > > ProcessNextEventFromQueue() when we are done processing+dispatching the event) > > If that is true, then it means if there's a hit-test in flight in > > EventDispatcher, the |in_flight_event_details_| would be non-null so it would > > already go inside this if-block without checking IsHitTestInFlight()? > > in_flight_event_details_ is only created from callbacks from EventDispatcher > (DispatchInputEventToWindow for one). So, if calling to EventDispatcher doesn't > immediately call to DispatchInputEventToWindow() in_flight_event_details_ won't > be set to a non-null value. I ended up keeping event_dispatcher_.IsHitTestInFlight() and notifying WindowManagerState when hit-tests are done in EventDispatcher. Now the queue for event targeting and event dispatching is in WindowManagerState and the queue for other hit-test queries is in EventDispatcher. PTAL, thanks!
https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.cc:49: hittest_callback = std::move(callback); move to member initializer. https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:146: void ProcessEvent(const ui::Event& event, AcceleratorMatchPhase match_phase); Update the description. https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:263: HitTestCallback callback); I understand you're doing this as an initial step in the direction of adding async processing, but the heavy use of callbacks here makes this hard to follow. Won't we end up with a real class that likely has a delegate? Would it better to structure the code like that now? For example, I'm assuming the real hit test class is likely to coalesce multiple requests in some cases, or possibly end up synchronously processing. It's hard to see that now. I recommend adding a real class now for the hit-testing that has a delegate. You can make this class pure virtual if you want with two implementations. One that processes everything async, then other sync. Does that make sense? https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher_delegate.h (right): https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher_delegate.h:66: virtual void ProcessNextEventFromQueue() = 0; Add description.
Description was changed from ========== Make event-targeting asynchronous in window server. Change hit-testing in EventDispatcher to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=mus_ws_unittests ========== to ========== Make event-targeting asynchronous in window server. Change hit-testing in EventTargeter to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=ui_service_unittests ==========
Description was changed from ========== Make event-targeting asynchronous in window server. Change hit-testing in EventTargeter to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=ui_service_unittests ========== to ========== Make event-targeting asynchronous in window server. Change hit-testing in EventTargeter to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=service_unittest ==========
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.cc:49: hittest_callback = std::move(callback); On 2017/05/22 16:06:56, sky wrote: > move to member initializer. Done. https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:146: void ProcessEvent(const ui::Event& event, AcceleratorMatchPhase match_phase); On 2017/05/22 16:06:56, sky wrote: > Update the description. Added that it may be asynchronous. https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher.h:263: HitTestCallback callback); On 2017/05/22 16:06:56, sky wrote: > I understand you're doing this as an initial step in the direction of adding > async processing, but the heavy use of callbacks here makes this hard to follow. > Won't we end up with a real class that likely has a delegate? Would it better to > structure the code like that now? For example, I'm assuming the real hit test > class is likely to coalesce multiple requests in some cases, or possibly end up > synchronously processing. It's hard to see that now. > > I recommend adding a real class now for the hit-testing that has a delegate. You > can make this class pure virtual if you want with two implementations. One that > processes everything async, then other sync. Does that make sense? As discussed, changed to use the new EventTargeter class (introduced in https://codereview.chromium.org/2905333002) to make the code a bit cleaner. https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... File services/ui/ws/event_dispatcher_delegate.h (right): https://codereview.chromium.org/2884463002/diff/80001/services/ui/ws/event_di... services/ui/ws/event_dispatcher_delegate.h:66: virtual void ProcessNextEventFromQueue() = 0; On 2017/05/22 16:06:56, sky wrote: > Add description. Done.
https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:235: void EventDispatcher::UpdateCursorProviderByLastKnownLocation() { This class is mostly written assuming synchronous processing. If we're going to introduce async processing I think we need to better restructure it. For example, this function may be called after you've gotten the result from ProcessPointerEventOnFoundTarget. We should ensure there is at most one async lookup when processing an event. Additionally for some event processing we do not need to lookup the target at all, the way you have it now assumes we do. I think the code should be structured to only do lookup if necessary, and if lookup is needed, we ensure it is done only once. https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:198: DeepestWindow deepest_window); const DeepestWindow& here and bove? https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:75: HitTestRequest(gfx::Point* location, I'm nervous about keeping pointers to members from another class that are asynchronously processed. That's a recipe for use after free and hard to reason about. Can these be by value and if needed pass them to the callback?
On 2017/05/30 17:26:02, sky wrote: > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > File services/ui/ws/event_dispatcher.cc (right): > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > services/ui/ws/event_dispatcher.cc:235: void > EventDispatcher::UpdateCursorProviderByLastKnownLocation() { > This class is mostly written assuming synchronous processing. If we're going to > introduce async processing I think we need to better restructure it. For > example, this function may be called after you've gotten the result from > ProcessPointerEventOnFoundTarget. We should ensure there is at most one async > lookup when processing an event. Additionally for some event processing we do > not need to lookup the target at all, the way you have it now assumes we do. > > I think the code should be structured to only do lookup if necessary, and if > lookup is needed, we ensure it is done only once. > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > File services/ui/ws/event_dispatcher.h (right): > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > services/ui/ws/event_dispatcher.h:198: DeepestWindow deepest_window); > const DeepestWindow& here and bove? > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... > File services/ui/ws/event_targeter.h (right): > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... > services/ui/ws/event_targeter.h:75: HitTestRequest(gfx::Point* location, > I'm nervous about keeping pointers to members from another class that are > asynchronously processed. That's a recipe for use after free and hard to reason > about. Can these be by value and if needed pass them to the callback? Ria, I'm wondering if it's possible to have a function like: bool WillProcessingRequireWindowLookup(const ui::Event& event); On EventDispatcher? If that returns true, then the caller has to lookup the location and supply it to processevent. This way EventDispatcher doesn't need the async handling, it's moved out to a separate class. WDYT?
On 2017/05/30 17:40:40, sky wrote: > On 2017/05/30 17:26:02, sky wrote: > > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > > File services/ui/ws/event_dispatcher.cc (right): > > > > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > > services/ui/ws/event_dispatcher.cc:235: void > > EventDispatcher::UpdateCursorProviderByLastKnownLocation() { > > This class is mostly written assuming synchronous processing. If we're going > to > > introduce async processing I think we need to better restructure it. For > > example, this function may be called after you've gotten the result from > > ProcessPointerEventOnFoundTarget. We should ensure there is at most one async > > lookup when processing an event. Additionally for some event processing we do > > not need to lookup the target at all, the way you have it now assumes we do. > > > > I think the code should be structured to only do lookup if necessary, and if > > lookup is needed, we ensure it is done only once. > > > > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > > File services/ui/ws/event_dispatcher.h (right): > > > > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... > > services/ui/ws/event_dispatcher.h:198: DeepestWindow deepest_window); > > const DeepestWindow& here and bove? > > > > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... > > File services/ui/ws/event_targeter.h (right): > > > > > https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... > > services/ui/ws/event_targeter.h:75: HitTestRequest(gfx::Point* location, > > I'm nervous about keeping pointers to members from another class that are > > asynchronously processed. That's a recipe for use after free and hard to > reason > > about. Can these be by value and if needed pass them to the callback? > > Ria, > > I'm wondering if it's possible to have a function like: > > bool WillProcessingRequireWindowLookup(const ui::Event& event); > > On EventDispatcher? If that returns true, then the caller has to lookup the > location and supply it to processevent. This way EventDispatcher doesn't need > the async handling, it's moved out to a separate class. WDYT? There are three places that need to handle async targeting: 1. EventDispatcher::ProcessPointerEvent 2. EventDispatcher::UpdateNonClientAreaForCurrentWindow 3. EventDispatcher::UpdateCursorProviderByLastKnownLocation 2 and 3 can also be called when the window hierarchy/bounds/etc change even when we don't have an event coming in. If we add WillProcessingRequireWindowLookup, then for 1, WindowManagerState::ProcessEventImpl would then check EventDispatcher::WillProcessingRequireWindowLookup and ask EventTargeter to find the target and callback to EventDispatcher::ProcessPointerEvent directly? 2 is called by WindowServer so WindowServer can do what WindowManagerState did for 1 or ask WindowManagerState to do that. But several places in EventDispatcher also call 3, for example EventDispatcher::CancelPointerEventsToTarget which is called when widnow hierarchy/visibility/etc changes, EventDispatcher still needs to deal with async in these cases? Did I misunderstand sth?
*SIGH* What I want to make sure is that we avoid repeated async lookup during processing the same event. Only one async lookup during processing a particular event is necessary. How do you propose we deal with that given EventDispatcher may internally trigger calls that are async during processing? On Wed, May 31, 2017 at 12:38 PM, <riajiang@chromium.org> wrote: > On 2017/05/30 17:40:40, sky wrote: > > On 2017/05/30 17:26:02, sky wrote: > > > > > > https://codereview.chromium.org/2884463002/diff/120001/ > services/ui/ws/event_dispatcher.cc > > > File services/ui/ws/event_dispatcher.cc (right): > > > > > > > > > https://codereview.chromium.org/2884463002/diff/120001/ > services/ui/ws/event_dispatcher.cc#newcode235 > > > services/ui/ws/event_dispatcher.cc:235: void > > > EventDispatcher::UpdateCursorProviderByLastKnownLocation() { > > > This class is mostly written assuming synchronous processing. If we're > going > > to > > > introduce async processing I think we need to better restructure it. > For > > > example, this function may be called after you've gotten the result > from > > > ProcessPointerEventOnFoundTarget. We should ensure there is at most > one > async > > > lookup when processing an event. Additionally for some event > processing we > do > > > not need to lookup the target at all, the way you have it now assumes > we do. > > > > > > I think the code should be structured to only do lookup if necessary, > and if > > > lookup is needed, we ensure it is done only once. > > > > > > > > > https://codereview.chromium.org/2884463002/diff/120001/ > services/ui/ws/event_dispatcher.h > > > File services/ui/ws/event_dispatcher.h (right): > > > > > > > > > https://codereview.chromium.org/2884463002/diff/120001/ > services/ui/ws/event_dispatcher.h#newcode198 > > > services/ui/ws/event_dispatcher.h:198: DeepestWindow deepest_window); > > > const DeepestWindow& here and bove? > > > > > > > > > https://codereview.chromium.org/2884463002/diff/120001/ > services/ui/ws/event_targeter.h > > > File services/ui/ws/event_targeter.h (right): > > > > > > > > > https://codereview.chromium.org/2884463002/diff/120001/ > services/ui/ws/event_targeter.h#newcode75 > > > services/ui/ws/event_targeter.h:75: HitTestRequest(gfx::Point* > location, > > > I'm nervous about keeping pointers to members from another class that > are > > > asynchronously processed. That's a recipe for use after free and hard > to > > reason > > > about. Can these be by value and if needed pass them to the callback? > > > > Ria, > > > > I'm wondering if it's possible to have a function like: > > > > bool WillProcessingRequireWindowLookup(const ui::Event& event); > > > > On EventDispatcher? If that returns true, then the caller has to lookup > the > > location and supply it to processevent. This way EventDispatcher doesn't > need > > the async handling, it's moved out to a separate class. WDYT? > > There are three places that need to handle async targeting: > 1. EventDispatcher::ProcessPointerEvent > 2. EventDispatcher::UpdateNonClientAreaForCurrentWindow > 3. EventDispatcher::UpdateCursorProviderByLastKnownLocation > 2 and 3 can also be called when the window hierarchy/bounds/etc change even > when we don't have an event coming in. > > If we add WillProcessingRequireWindowLookup, then for 1, > WindowManagerState::ProcessEventImpl would then check > EventDispatcher::WillProcessingRequireWindowLookup and ask EventTargeter > to find the target and callback to EventDispatcher::ProcessPointerEvent > directly? > > 2 is called by WindowServer so WindowServer can do what > WindowManagerState did for 1 or ask WindowManagerState to do that. > > But several places in EventDispatcher also call 3, for example > EventDispatcher::CancelPointerEventsToTarget which is called when widnow > hierarchy/visibility/etc changes, EventDispatcher still needs to deal > with async in these cases? Did I misunderstand sth? > > https://codereview.chromium.org/2884463002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #7 (id:140001) has been deleted
On 2017/05/31 21:26:43, sky wrote: > *SIGH* > What I want to make sure is that we avoid repeated async lookup during > processing the same event. Only one async lookup during processing a > particular event is necessary. How do you propose we deal with that given > EventDispatcher may internally trigger calls that are async during > processing? I think the UpdateCursorProviderByLastKnownLocation call in EventDispatcher::ProcessPointerEventOnFoundTarget is the only place that EventDispatcher internally triggers async calls during processing. Other places that EventDispatcher trigger calls to UpdateCursorProviderByLastKnownLocation are when capture/ window hierarchy/ window visibility etc. changes. I changed the first case to use the target we just found (replied to the comment about caching). WDYT? https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:235: void EventDispatcher::UpdateCursorProviderByLastKnownLocation() { On 2017/05/30 17:26:02, sky wrote: > This class is mostly written assuming synchronous processing. If we're going to > introduce async processing I think we need to better restructure it. For > example, this function may be called after you've gotten the result from > ProcessPointerEventOnFoundTarget. We should ensure there is at most one async > lookup when processing an event. Additionally for some event processing we do > not need to lookup the target at all, the way you have it now assumes we do. > > I think the code should be structured to only do lookup if necessary, and if > lookup is needed, we ensure it is done only once. True! I didn't notice that UpdateCursorProviderByLastKnownLocation is called right after we found the new target in ProcessPointerEventOnFoundTarget. For now, I changed it to be still always look up the target when processing event but use the target we found for UpdateCursorProviderByLastKnownLocationWithWindow so we only do async targeting once for an event. As Sadrul mentioned in a previous comment, after the HittestComponent is done we are going to look into caching to see if targeting is necessary for both event processing and window hierarchy/bounds changes (when UpdateCursorProviderByLastKnownLocation would be called). WDYT? https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:198: DeepestWindow deepest_window); On 2017/05/30 17:26:02, sky wrote: > const DeepestWindow& here and bove? Done. https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/120001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:75: HitTestRequest(gfx::Point* location, On 2017/05/30 17:26:02, sky wrote: > I'm nervous about keeping pointers to members from another class that are > asynchronously processed. That's a recipe for use after free and hard to reason > about. Can these be by value and if needed pass them to the callback? Makes sense, changed to by value and return the updated location/display_id.
Ok, you convinced me to continue with what you have. I have a question below as to whether we need to support more than one hit-test request at a time. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:227: event_targeter_->FindDeepestVisibleWindowForEvents( Can all the calls to async code DCHECK if there is an async call in flight? https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:325: if (new_mouse_location != mouse_pointer_last_location_) Is there a reason not to set the values all the time? That is, get rid of the if and always set? https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:359: event_targeter_->PointerTargetForEvent( As this is a single line now (albeit a long single line) how about moving to the single place that calls this function? https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:163: // OnMouseCursorLocationChanged since mouse location would still be the same 'would still be' -> is https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:192: const gfx::Point& new_location, Please document what the args are. optional: rename new_location to location_in_display and new_display_id to display_id. This comment applies to all new functions. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:85: gfx::Point hittest_location; Be consistent. You have HitTest and yet these are called hittest. I would expect hit_test here. That said, I think hittest (or hit_test) is redundant given the structure this is in. I recommend removing hittest_ entirely here. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:105: HitTestCallback hittest_callback_; hit_test https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:108: bool hittest_in_flight_; hit_test https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:115: std::queue<std::unique_ptr<HitTestRequest>> hittest_request_queue_; hit_test
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:227: event_targeter_->FindDeepestVisibleWindowForEvents( On 2017/05/31 23:36:34, sky wrote: > Can all the calls to async code DCHECK if there is an async call in flight? In EventTargeter::FindDeepestVisibleWindowForEvents, |location| is going to be put in the request queue if there's a hit-test in flight, and then in EventTargeter::FindDeepestVisibleWindowForEventsImpl there's a DCHECK before we start async targeting. Were you thinking sth like this? https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:325: if (new_mouse_location != mouse_pointer_last_location_) On 2017/05/31 23:36:34, sky wrote: > Is there a reason not to set the values all the time? That is, get rid of the if > and always set? Removed if. I was thinking that we don't have to set if values are the same? In general, do we only check that if it's an expensive assignment? https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:359: event_targeter_->PointerTargetForEvent( On 2017/05/31 23:36:34, sky wrote: > As this is a single line now (albeit a long single line) how about moving to the > single place that calls this function? True, moved to ProcessEvent. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:163: // OnMouseCursorLocationChanged since mouse location would still be the same On 2017/05/31 23:36:34, sky wrote: > 'would still be' -> is Done. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:192: const gfx::Point& new_location, On 2017/05/31 23:36:34, sky wrote: > Please document what the args are. > optional: rename new_location to location_in_display and new_display_id to > display_id. > > This comment applies to all new functions. Done. And renamed. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:85: gfx::Point hittest_location; On 2017/05/31 23:36:35, sky wrote: > Be consistent. You have HitTest and yet these are called hittest. I would expect > hit_test here. That said, I think hittest (or hit_test) is redundant given the > structure this is in. I recommend removing hittest_ entirely here. Make sense, removed hittest_. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:105: HitTestCallback hittest_callback_; On 2017/05/31 23:36:35, sky wrote: > hit_test Done. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:108: bool hittest_in_flight_; On 2017/05/31 23:36:34, sky wrote: > hit_test Done. https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:115: std::queue<std::unique_ptr<HitTestRequest>> hittest_request_queue_; On 2017/05/31 23:36:34, sky wrote: > hit_test Done.
https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:227: event_targeter_->FindDeepestVisibleWindowForEvents( On 2017/06/01 18:22:11, riajiang wrote: > On 2017/05/31 23:36:34, sky wrote: > > Can all the calls to async code DCHECK if there is an async call in flight? > > In EventTargeter::FindDeepestVisibleWindowForEvents, |location| is going to be > put in the request queue if there's a hit-test in flight, and then in > EventTargeter::FindDeepestVisibleWindowForEventsImpl there's a DCHECK before we > start async targeting. Were you thinking sth like this? My question is how do we end up in EventTargeter::FindDeepestVisibleWindowForEvents() with an async event already in the queue? In an earlier thread we agreed there should be at most one async request per event.
https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/160001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:227: event_targeter_->FindDeepestVisibleWindowForEvents( On 2017/06/01 21:12:58, sky wrote: > On 2017/06/01 18:22:11, riajiang wrote: > > On 2017/05/31 23:36:34, sky wrote: > > > Can all the calls to async code DCHECK if there is an async call in flight? > > > > In EventTargeter::FindDeepestVisibleWindowForEvents, |location| is going to be > > put in the request queue if there's a hit-test in flight, and then in > > EventTargeter::FindDeepestVisibleWindowForEventsImpl there's a DCHECK before > we > > start async targeting. Were you thinking sth like this? > > My question is how do we end up in > EventTargeter::FindDeepestVisibleWindowForEvents() with an async event already > in the queue? In an earlier thread we agreed there should be at most one async > request per event. EventTargeter::FindDeepestVisibleWindowForEvents() can also be called from EventDispatcher::UpdateNonClientAreaForCurrentWindow() and EventDispatcher::UpdateCursorProviderByLastKnownLocation() when things like window hierarchy and bounds changed without an input event, so FindDeepestVisibleWindowForEvents() can be called when there's a hit-test in flight already. I think it would be hard to DCHECK that there's at most one async request per event because we can be handling an input event and receiving window hierarchy change request at the same time? (I can rename FindDeepestVisibleWindowForEvents to FindDeepestVisibleWindowForLocation?)
On Thu, Jun 1, 2017 at 3:00 PM, <riajiang@chromium.org> wrote: > > https://codereview.chromium.org/2884463002/diff/160001/ > services/ui/ws/event_dispatcher.cc > File services/ui/ws/event_dispatcher.cc (right): > > https://codereview.chromium.org/2884463002/diff/160001/ > services/ui/ws/event_dispatcher.cc#newcode227 > services/ui/ws/event_dispatcher.cc:227: > event_targeter_->FindDeepestVisibleWindowForEvents( > On 2017/06/01 21:12:58, sky wrote: > > On 2017/06/01 18:22:11, riajiang wrote: > > > On 2017/05/31 23:36:34, sky wrote: > > > > Can all the calls to async code DCHECK if there is an async call > in flight? > > > > > > In EventTargeter::FindDeepestVisibleWindowForEvents, |location| is > going to be > > > put in the request queue if there's a hit-test in flight, and then > in > > > EventTargeter::FindDeepestVisibleWindowForEventsImpl there's a > DCHECK before > > we > > > start async targeting. Were you thinking sth like this? > > > > My question is how do we end up in > > EventTargeter::FindDeepestVisibleWindowForEvents() with an async event > already > > in the queue? In an earlier thread we agreed there should be at most > one async > > request per event. > > EventTargeter::FindDeepestVisibleWindowForEvents() can also be called > from EventDispatcher::UpdateNonClientAreaForCurrentWindow() and > EventDispatcher::UpdateCursorProviderByLastKnownLocation() when things > like window hierarchy and bounds changed without an input event, so > FindDeepestVisibleWindowForEvents() can be called when there's a > hit-test in flight already. > That makes sense, thanks. > I think it would be hard to DCHECK that > there's at most one async request per event because we can be handling > an input event and receiving window hierarchy change request at the same > time? > For the most part mus-ws is single threaded, so we can't be processing both an input event and hierarchy change at the same time. Did you verify we don't end up calling UpdateNonClientForCurrentWindow and/or UpdateCursorProviderByLastKnownLocation multiple times during processing a call via mojom? Ideally when processing any mojom function call we would make a note that we need to run any of these functions, and process it after the mojom function call. That way we would ensure we don't unnecessarily queue up multiple queries for the same location. I'm not sure something like this is possible though. I'll ask Yuhzu later today. -Scott > > (I can rename FindDeepestVisibleWindowForEvents to > FindDeepestVisibleWindowForLocation?) > > https://codereview.chromium.org/2884463002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> For the most part mus-ws is single threaded, so we can't be processing both > an input event and hierarchy change at the same time. > > Did you verify we don't end up calling UpdateNonClientForCurrentWindow > and/or UpdateCursorProviderByLastKnownLocation multiple times during > processing a call via mojom? Ideally when processing any mojom function > call we would make a note that we need to run any of these functions, and > process it after the mojom function call. That way we would ensure we don't > unnecessarily queue up multiple queries for the same location. I'm not sure > something like this is possible though. I'll ask Yuhzu later today. > > -Scott I'm a bit confused about how to verify this via mojom. When we call UpdateNonClientForCurrentWindow or UpdateCursorProviderByLastKnownLocation after window hierarchy/ bounds changes, WindowTree creates an Operation object (e.g. https://cs.chromium.org/chromium/src/services/ui/ws/window_tree.cc?l=540) although I'm not sure if this is being used to ensure mus-ws only handle one operation at a time or just to avoid notifications generated by suboperations in the window server (documentation about that https://cs.chromium.org/chromium/src/services/ui/ws/operation.h) For ProcessEvent tho, I don't think it's called via mojom - it came from DrmWindowHost::DispatchEvent -> PlatformDisplayDefault::DispatchEvent -> Display::OnEventFromSource -> WindowManagerState -> ...?
On Fri, Jun 2, 2017 at 10:56 AM, <riajiang@chromium.org> wrote: > > For the most part mus-ws is single threaded, so we can't be processing > both > > an input event and hierarchy change at the same time. > > > > Did you verify we don't end up calling UpdateNonClientForCurrentWindow > > and/or UpdateCursorProviderByLastKnownLocation multiple times during > > processing a call via mojom? Ideally when processing any mojom function > > call we would make a note that we need to run any of these functions, and > > process it after the mojom function call. That way we would ensure we > don't > > unnecessarily queue up multiple queries for the same location. I'm not > sure > > something like this is possible though. I'll ask Yuhzu later today. > > > > -Scott > > > I'm a bit confused about how to verify this via mojom. > Ya, sorry, I was suggesting you look at the functions that can be called via the mojom and verify they don't result in multiple calls to EventDispatcher. I realize that's too painful and error prone though, so ignore it. As you mention below, in general we create an Operation for all mojom calls. Rather than calling directly to EventDispatchers UpdateNonClientForCurrentWindow and UpdateCursorProviderByLastKnownLocation we could note it in the current Operation and then when the operation goes out of scope process the request. That would ensure we only do one request per operation. I spoke with Ken and he said it would be possible to add first class support for something like this to bindings so that we don't need to roll our own. But, I don't want to hold you up on that. So, let me take another look. -Scott > When we call > UpdateNonClientForCurrentWindow or UpdateCursorProviderByLastKnownLocation > > after window hierarchy/ bounds changes, WindowTree creates an Operation > object > (e.g. https://cs.chromium.org/chromium/src/services/ui/ws/ > window_tree.cc?l=540) > although I'm not sure if this is being used to ensure mus-ws only handle > one > operation at a time or just to avoid notifications generated by > suboperations > in the window server (documentation about that > https://cs.chromium.org/chromium/src/services/ui/ws/operation.h) > For ProcessEvent tho, I don't think it's called via mojom > - it came from DrmWindowHost::DispatchEvent -> > PlatformDisplayDefault::DispatchEvent -> Display::OnEventFromSource -> > WindowManagerState -> ...? > > https://codereview.chromium.org/2884463002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:461: event_targeter_->ProcessNextHittesetRequestFromQueue(); Why does this code need to call back to event_targeter_? Can't EventTargeter take care of this? By that I mean EventTargeter would call the callback, and then process the next event as appropriate. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:461: event_targeter_->ProcessNextHittesetRequestFromQueue(); Because this code is now processed async, I think you need to propagate the check for mouse_cursor_source_window_ here. By that I mean early out if !mouse_cursor_source_window_. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:480: void EventDispatcher::UpdateCursorProviderByLastKnownLocationWithWindow( It's not clear what 'WithWindow' means here. Maybe UpdateCursorProviderByLastKnownLocationImpl? https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:484: SetMouseCursorSourceWindow(deepest_window.window); Similar comment here about early out if mouse_button_down_. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:516: const PointerTarget& pointer_target_found) { pointer_target_found -> pointer_target https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:168: void UpdateMousePointerLocation(const gfx::Point& new_mouse_location, Update -> Set i.e. SetMousePointerLocation. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:169: const int64_t new_mouse_display_id); Generally we don't use const for primitive types.
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:461: event_targeter_->ProcessNextHittesetRequestFromQueue(); On 2017/06/02 21:17:51, sky wrote: > Because this code is now processed async, I think you need to propagate the > check for mouse_cursor_source_window_ here. By that I mean early out if > !mouse_cursor_source_window_. Yes! Done. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:461: event_targeter_->ProcessNextHittesetRequestFromQueue(); On 2017/06/02 21:17:51, sky wrote: > Why does this code need to call back to event_targeter_? Can't EventTargeter > take care of this? By that I mean EventTargeter would call the callback, and > then process the next event as appropriate. I was thinking ui::ws::FindDeepestVisibleWindowForEvents used in the callback run in EventTargeter::FindDeepestVisibleWindowForEventsAsync will be async when it's changed to be calling Blink so we only know it's really done when these functions in EventDispatcher are called. But Blink can call a callback back to EventTargeter and then we can process the next event and deliver target back to EventDispatcher. Moved to EventTargeter. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:480: void EventDispatcher::UpdateCursorProviderByLastKnownLocationWithWindow( On 2017/06/02 21:17:51, sky wrote: > It's not clear what 'WithWindow' means here. Maybe > UpdateCursorProviderByLastKnownLocationImpl? Since event_targeter_->ProcessNextHittesetRequestFromQueue(); is now deleted, removed this function. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:484: SetMouseCursorSourceWindow(deepest_window.window); On 2017/06/02 21:17:51, sky wrote: > Similar comment here about early out if mouse_button_down_. Done. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:516: const PointerTarget& pointer_target_found) { On 2017/06/02 21:17:51, sky wrote: > pointer_target_found -> pointer_target Done. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:168: void UpdateMousePointerLocation(const gfx::Point& new_mouse_location, On 2017/06/02 21:17:51, sky wrote: > Update -> Set i.e. SetMousePointerLocation. Done. https://codereview.chromium.org/2884463002/diff/200001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:169: const int64_t new_mouse_display_id); On 2017/06/02 21:17:51, sky wrote: > Generally we don't use const for primitive types. I see, removed.
I apologize for the multiple rounds of comments here, but this is tricky! I think it's close! https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:65: const EventTargeter* event_targeter() const { return event_targeter_.get(); } EventTargeter is an implementation detail of EventDispatcher and we shouldn't need to expose it. Add a IsProcessingEvent() here that WindowManagerState calls to. https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... File services/ui/ws/event_targeter.cc (right): https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:40: target_callback_ = std::move(callback); Why do you need target_callback_ and hit_test_callback_ as members? You should be able to pass them as bound members to bind such that PointerTargetForEventOnFoundWindow gets the callback. Use base::Passed(&callback). https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:64: if (hit_test_in_flight_ || !hit_test_request_queue_.empty()) Change this if to a return and get rid of the other returns, e.g. return hit_test_in_flight || ... https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:26: struct PointerTarget { Sorry for not realizing this sooner, but PointerTarget is specific to event dispatching and shouldn't have been moved here. Specifically is_mouse_event and is_pointer_down are state only useful for EventDispatcher. Both of the functions you have (PointerTargetForEvent and FindDeepestVisibleWindowForLocation) are the exact same thing, (and you have one calling the other). How about a single function called GetEventTarget or FindTarget? The callback can take a structure if you want, but not both DeepestWindow and other information.
https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:65: const EventTargeter* event_targeter() const { return event_targeter_.get(); } On 2017/06/02 23:28:20, sky wrote: > EventTargeter is an implementation detail of EventDispatcher and we shouldn't > need to expose it. Add a IsProcessingEvent() here that WindowManagerState calls > to. True, done. https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... File services/ui/ws/event_targeter.cc (right): https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:40: target_callback_ = std::move(callback); On 2017/06/02 23:28:20, sky wrote: > Why do you need target_callback_ and hit_test_callback_ as members? You should > be able to pass them as bound members to bind such that > PointerTargetForEventOnFoundWindow gets the callback. Use > base::Passed(&callback). I was passing them around (with std::move), but after separating out EventTargeter I thought adding them as members would avoid having to pass them around... Changed to base::Passed(&callback). https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:64: if (hit_test_in_flight_ || !hit_test_request_queue_.empty()) On 2017/06/02 23:28:20, sky wrote: > Change this if to a return and get rid of the other returns, e.g. > return hit_test_in_flight || ... Done. https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/240001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:26: struct PointerTarget { On 2017/06/02 23:28:20, sky wrote: > Sorry for not realizing this sooner, but PointerTarget is specific to event > dispatching and shouldn't have been moved here. Specifically is_mouse_event and > is_pointer_down are state only useful for EventDispatcher. Both of the functions > you have (PointerTargetForEvent and FindDeepestVisibleWindowForLocation) are the > exact same thing, (and you have one calling the other). How about a single > function called GetEventTarget or FindTarget? The callback can take a structure > if you want, but not both DeepestWindow and other information. Ah I see, makes sense. Moved PointerTarget back to EventDispatcher and added LocationTarget struct for the HitTestCallback return type. Changed to only have FindTargetForLocation.
Rebased. Could you take another look? Thanks!
https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:309: (event.AsPointerEvent())->root_location(), event_display_id_, Looks like you have unnecessary () around event.AsPointerEvent() https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... File services/ui/ws/event_targeter.cc (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:50: void EventTargeter::FindTargetForLocationImpl(const gfx::Point& location, These names are rather confusing. I'm hoping that is temporary until hooked up to the real hittest callback. In the mean time how about naming this ProcessFindTarget() and FindTargetForLocationAsync to FindTargetForLocationNow() ? https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:83: base::ResetAndReturn(&callback).Run(location_target); std::move(callback).Run(...); Generally you only need to use ResetAndReturn if you need to ensure a member variable is reset before running the callback. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:66: void ProcessNextHittesetRequestFromQueue(); HitTestRequest https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:73: // Request would go into this queue if it's coming from EventDispatcher:: Update description. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/test_ut... File services/ui/ws/test_utils.h (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/test_ut... services/ui/ws/test_utils.h:241: bool event_queue_empty() { return wms_->event_queue_.empty(); } is_event_queue_empty() const
https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:309: (event.AsPointerEvent())->root_location(), event_display_id_, On 2017/06/07 17:05:07, sky wrote: > Looks like you have unnecessary () around event.AsPointerEvent() Done. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... File services/ui/ws/event_targeter.cc (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:50: void EventTargeter::FindTargetForLocationImpl(const gfx::Point& location, On 2017/06/07 17:05:07, sky wrote: > These names are rather confusing. I'm hoping that is temporary until hooked up > to the real hittest callback. In the mean time how about naming this > ProcessFindTarget() and FindTargetForLocationAsync to FindTargetForLocationNow() > ? Sure sg, done. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.cc:83: base::ResetAndReturn(&callback).Run(location_target); On 2017/06/07 17:05:07, sky wrote: > std::move(callback).Run(...); > Generally you only need to use ResetAndReturn if you need to ensure a member > variable is reset before running the callback. I see, done. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:66: void ProcessNextHittesetRequestFromQueue(); On 2017/06/07 17:05:07, sky wrote: > HitTestRequest Done. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:73: // Request would go into this queue if it's coming from EventDispatcher:: On 2017/06/07 17:05:07, sky wrote: > Update description. This is still valid? Even though we call FindTargetForLocation from EventDispatcher::ProcessEvent, events are still going to WindowManagerState::event_queue_ if there's an active hit-test cuz we check that in WindowManagerState::ProcessEvent? https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/test_ut... File services/ui/ws/test_utils.h (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/test_ut... services/ui/ws/test_utils.h:241: bool event_queue_empty() { return wms_->event_queue_.empty(); } On 2017/06/07 17:05:07, sky wrote: > is_event_queue_empty() const Done.
LGTM with a better description as mentioned below. https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:73: // Request would go into this queue if it's coming from EventDispatcher:: On 2017/06/07 18:15:04, riajiang wrote: > On 2017/06/07 17:05:07, sky wrote: > > Update description. > > This is still valid? Even though we call FindTargetForLocation from > EventDispatcher::ProcessEvent, events are still going to > WindowManagerState::event_queue_ if there's an active hit-test cuz we check that > in WindowManagerState::ProcessEvent? This description is entirely in terms of other classes, which is confusing. I was thinking of something like: Requests for a new location while waiting on an existing request are added here.
nice! lgtm with some nits. https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:228: base::Unretained(this))); I guess using base::Unretained here (and below) is safe because EventDispatcher owns |event_targeter_|? https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:146: bool IsProcessingEvent(); const method. https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_delegate.h (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_delegate.h:71: virtual void ProcessNextEventFromQueue() = 0; Instead of saying FromQueue, maybe 'ProcessNextAvailableEvent' or something like that? https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:260: void EventDispatcherTest::RunTasks() { Because you are calling this only after calling dispatcher()->ProcessEvent(), can you instead introduce a 'DispatchEvent()' function in the test, that does both?
https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... File services/ui/ws/event_targeter.h (right): https://codereview.chromium.org/2884463002/diff/300001/services/ui/ws/event_t... services/ui/ws/event_targeter.h:73: // Request would go into this queue if it's coming from EventDispatcher:: On 2017/06/07 19:46:41, sky wrote: > On 2017/06/07 18:15:04, riajiang wrote: > > On 2017/06/07 17:05:07, sky wrote: > > > Update description. > > > > This is still valid? Even though we call FindTargetForLocation from > > EventDispatcher::ProcessEvent, events are still going to > > WindowManagerState::event_queue_ if there's an active hit-test cuz we check > that > > in WindowManagerState::ProcessEvent? > > This description is entirely in terms of other classes, which is confusing. I > was thinking of something like: > Requests for a new location while waiting on an existing request are added here. Oh I see, done. https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:228: base::Unretained(this))); On 2017/06/07 20:16:04, sadrul wrote: > I guess using base::Unretained here (and below) is safe because EventDispatcher > owns |event_targeter_|? Yes so |this| still exists when the callback is run. https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:146: bool IsProcessingEvent(); On 2017/06/07 20:16:04, sadrul wrote: > const method. Done. https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_delegate.h (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_delegate.h:71: virtual void ProcessNextEventFromQueue() = 0; On 2017/06/07 20:16:04, sadrul wrote: > Instead of saying FromQueue, maybe 'ProcessNextAvailableEvent' or something like > that? Changed to ProcessNextAvailableEvent. https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2884463002/diff/320001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:260: void EventDispatcherTest::RunTasks() { On 2017/06/07 20:16:04, sadrul wrote: > Because you are calling this only after calling dispatcher()->ProcessEvent(), > can you instead introduce a 'DispatchEvent()' function in the test, that does > both? It's also used after SetMousePointerDisplayLocation, so I moved RunTasks to private and added DispatchEvent and SetMousePointerDisplayLocation that do both.
The CQ bit was checked by riajiang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2884463002/#ps340001 (title: "comments")
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": 340001, "attempt_start_ts": 1496872008412590,
"parent_rev": "c2af5956cc783ccf67f93c3028f892ce489a2f28", "commit_rev":
"4cd9767ab7c89145bbec6b343ae701c83bf42e9d"}
Message was sent while issue was closed.
Description was changed from ========== Make event-targeting asynchronous in window server. Change hit-testing in EventTargeter to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=service_unittest ========== to ========== Make event-targeting asynchronous in window server. Change hit-testing in EventTargeter to be asynchronous to accommodate hit-testing cases that cannot be solved by HittestComponent if the flag "enable-async-event-targeting" is turned on. Next steps: 1. Explore hit-test optimization/caching. 2. Once HittestComponent (gklassen@) is implemented, update event targeting to talk to HittestComponent synchronously for most cases and talk to blink for "hard" cases that cannot be solved by HittestComponent. BUG=710016 TEST=service_unittest Review-Url: https://codereview.chromium.org/2884463002 Cr-Commit-Position: refs/heads/master@{#477783} Committed: https://chromium.googlesource.com/chromium/src/+/4cd9767ab7c89145bbec6b343ae7... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:340001) as https://chromium.googlesource.com/chromium/src/+/4cd9767ab7c89145bbec6b343ae7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
