Make event-targeting asynchronous in window server.

services/ui/ws/event_dispatcher.cc

Index: services/ui/ws/event_dispatcher.cc
diff --git a/services/ui/ws/event_dispatcher.cc b/services/ui/ws/event_dispatcher.cc
index c6e6897508347096389172210d6d3e9b91594479..89f4457ebce79703186bea3628513793c0f8b6a9 100644
--- a/services/ui/ws/event_dispatcher.cc
+++ b/services/ui/ws/event_dispatcher.cc
@@ -224,30 +224,21 @@ const ServerWindow* EventDispatcher::GetWindowForMouseCursor() const {
 
 void EventDispatcher::UpdateNonClientAreaForCurrentWindow() {
   if (mouse_cursor_source_window_) {
-    DeepestWindow deepest_window =
-        event_targeter_->FindDeepestVisibleWindowForEvents(
-            &mouse_pointer_last_location_, &mouse_pointer_display_id_);
-    if (deepest_window.window == mouse_cursor_source_window_) {
-      mouse_cursor_in_non_client_area_ = mouse_cursor_source_window_
-                                             ? deepest_window.in_non_client_area
-                                             : false;
-    }
+    event_targeter_->FindDeepestVisibleWindowForEvents(

[sky, 2017/05/31 23:36:34]

Can all the calls to async code DCHECK if there is an async call in flight?

[riajiang, 2017/06/01 18:22:11]

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?

[sky, 2017/06/01 21:12:58]

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.

[riajiang, 2017/06/01 22:00:21]

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?)

+        mouse_pointer_last_location_, mouse_pointer_display_id_,
+        base::BindOnce(
+            &EventDispatcher::UpdateNonClientAreaForCurrentWindowOnFoundWindow,
+            base::Unretained(this)));
   }
 }
 
 void EventDispatcher::UpdateCursorProviderByLastKnownLocation() {
   if (!mouse_button_down_) {
-    DeepestWindow deepest_window =
-        event_targeter_->FindDeepestVisibleWindowForEvents(
-            &mouse_pointer_last_location_, &mouse_pointer_display_id_);
-    SetMouseCursorSourceWindow(deepest_window.window);
-    if (mouse_cursor_source_window_) {
-      mouse_cursor_in_non_client_area_ = deepest_window.in_non_client_area;
-    } else {
-      SetMouseCursorSourceWindow(delegate_->GetRootWindowContaining(
-          &mouse_pointer_last_location_, &mouse_pointer_display_id_));
-      mouse_cursor_in_non_client_area_ = true;
-    }
+    event_targeter_->FindDeepestVisibleWindowForEvents(
+        mouse_pointer_last_location_, mouse_pointer_display_id_,
+        base::BindOnce(&EventDispatcher::
+                           UpdateCursorProviderByLastKnownLocationOnFoundWindow,
+                       base::Unretained(this)));
   }
 }
 
@@ -328,6 +319,15 @@ void EventDispatcher::SetMouseCursorSourceWindow(ServerWindow* window) {
     ObserveWindow(mouse_cursor_source_window_);
 }
 
+void EventDispatcher::UpdateMousePointerLocation(
+    const gfx::Point& new_mouse_location,
+    const int64_t new_mouse_display_id) {
+  if (new_mouse_location != mouse_pointer_last_location_)

[sky, 2017/05/31 23:36:34]

Is there a reason not to set the values all the time? That is, get rid of the if
and always set?

[riajiang, 2017/06/01 18:22:11]

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?

+    mouse_pointer_last_location_ = new_mouse_location;
+  if (new_mouse_display_id != mouse_pointer_display_id_)
+    mouse_pointer_display_id_ = new_mouse_display_id;
+}
+
 void EventDispatcher::ProcessKeyEvent(const ui::KeyEvent& event,
                                       AcceleratorMatchPhase match_phase) {
   Accelerator* post_target =
@@ -356,13 +356,31 @@ void EventDispatcher::ProcessKeyEvent(const ui::KeyEvent& event,
 
 void EventDispatcher::ProcessPointerEvent(const ui::PointerEvent& event) {
   DCHECK(event.IsPointerEvent());
+  event_targeter_->PointerTargetForEvent(

[sky, 2017/05/31 23:36:34]

As this is a single line now (albeit a long single line) how about moving to the
single place that calls this function?

[riajiang, 2017/06/01 18:22:11]

True, moved to ProcessEvent.

+      event, event_display_id_,
+      base::BindOnce(&EventDispatcher::ProcessPointerEventOnFoundTarget,
+                     base::Unretained(this), event));
+}
+
+void EventDispatcher::ProcessPointerEventOnFoundTarget(
+    const ui::PointerEvent& event,
+    const PointerTarget& pointer_target_found,
+    const DeepestWindow& deepest_window,
+    const gfx::Point& new_location,
+    const int64_t new_display_id) {
+  event_targeter_->ProcessNextHittesetRequestFromQueue();
+
+  std::unique_ptr<ui::Event> cloned_event = ui::Event::Clone(event);
+  if (new_display_id != event_display_id_) {
+    event_display_id_ = new_display_id;
+    cloned_event->AsLocatedEvent()->set_root_location(new_location);
+  }
+
   const bool is_mouse_event = event.IsMousePointerEvent();
 
   if (is_mouse_event) {
-    mouse_pointer_last_location_ = event.root_location();
-    mouse_pointer_display_id_ = event_display_id_;
-    delegate_->OnMouseCursorLocationChanged(event.root_location(),
-                                            event_display_id_);
+    UpdateMousePointerLocation(new_location, new_display_id);
+    delegate_->OnMouseCursorLocationChanged(new_location, new_display_id);
   }
 
   // Release capture on pointer up. For mouse we only release if there are
@@ -381,15 +399,15 @@ void EventDispatcher::ProcessPointerEvent(const ui::PointerEvent& event) {
   }
 
   if (drag_controller_) {
-    const PointerTarget target =
-        event_targeter_->PointerTargetForEvent(event, &event_display_id_);
-    if (drag_controller_->DispatchPointerEvent(event, target.window))
+    if (drag_controller_->DispatchPointerEvent(*cloned_event->AsPointerEvent(),
+                                               pointer_target_found.window))
       return;
   }
 
   if (capture_window_) {
     SetMouseCursorSourceWindow(capture_window_);
-    DispatchToClient(capture_window_, capture_window_client_id_, event);
+    DispatchToClient(capture_window_, capture_window_client_id_,
+                     *cloned_event->AsPointerEvent());
     return;
   }
 
@@ -397,7 +415,8 @@ void EventDispatcher::ProcessPointerEvent(const ui::PointerEvent& event) {
   if (!IsTrackingPointer(pointer_id) ||
       !pointer_targets_[pointer_id].is_pointer_down) {
     const bool any_pointers_down = AreAnyPointersDown();
-    UpdateTargetForPointer(pointer_id, event);
+    UpdateTargetForPointer(pointer_id, *cloned_event->AsPointerEvent(),
+                           pointer_target_found);
     if (is_mouse_event)
       SetMouseCursorSourceWindow(pointer_targets_[pointer_id].window);
 
@@ -410,7 +429,8 @@ void EventDispatcher::ProcessPointerEvent(const ui::PointerEvent& event) {
           delegate_->SetFocusedWindowFromEventDispatcher(pointer_target.window);
         ServerWindow* capture_window = pointer_target.window;
         if (!capture_window) {
-          gfx::Point event_location = event.root_location();
+          gfx::Point event_location =
+              cloned_event->AsPointerEvent()->root_location();
           int64_t event_display_id = event_display_id_;
           capture_window = delegate_->GetRootWindowContaining(
               &event_location, &event_display_id);
@@ -425,10 +445,13 @@ void EventDispatcher::ProcessPointerEvent(const ui::PointerEvent& event) {
   // up event to the window that had implicit capture. We have to set this
   // before we perform dispatch because the Delegate is going to read this
   // information from us.
-  if (is_pointer_going_up && is_mouse_event)
-    UpdateCursorProviderByLastKnownLocation();
+  if (is_pointer_going_up && is_mouse_event) {
+    UpdateCursorProviderByLastKnownLocationWithWindow(
+        deepest_window, new_location, new_display_id);
+  }
 
-  DispatchToPointerTarget(pointer_targets_[pointer_id], event);
+  DispatchToPointerTarget(pointer_targets_[pointer_id],
+                          *cloned_event->AsPointerEvent());
 
   if (is_pointer_going_up) {
     if (is_mouse_event)
@@ -457,22 +480,21 @@ void EventDispatcher::StopTrackingPointer(int32_t pointer_id) {
     UnobserveWindow(window);
 }
 
-void EventDispatcher::UpdateTargetForPointer(int32_t pointer_id,
-                                             const ui::LocatedEvent& event) {
+void EventDispatcher::UpdateTargetForPointer(
+    int32_t pointer_id,
+    const ui::PointerEvent& event,
+    const PointerTarget& pointer_target_found) {
   if (!IsTrackingPointer(pointer_id)) {
-    StartTrackingPointer(pointer_id, event_targeter_->PointerTargetForEvent(
-                                         event, &event_display_id_));
+    StartTrackingPointer(pointer_id, pointer_target_found);
     return;
   }
 
-  const PointerTarget pointer_target =
-      event_targeter_->PointerTargetForEvent(event, &event_display_id_);
-  if (pointer_target.window == pointer_targets_[pointer_id].window &&
-      pointer_target.in_nonclient_area ==
+  if (pointer_target_found.window == pointer_targets_[pointer_id].window &&
+      pointer_target_found.in_nonclient_area ==
           pointer_targets_[pointer_id].in_nonclient_area) {
     // The targets are the same, only set the down state to true if necessary.
     // Down going to up is handled by ProcessLocatedEvent().
-    if (pointer_target.is_pointer_down)
+    if (pointer_target_found.is_pointer_down)
       pointer_targets_[pointer_id].is_pointer_down = true;
     return;
   }
@@ -491,7 +513,46 @@ void EventDispatcher::UpdateTargetForPointer(int32_t pointer_id,
   // Technically we're updating in place, but calling start then stop makes for
   // simpler code.
   StopTrackingPointer(pointer_id);
-  StartTrackingPointer(pointer_id, pointer_target);
+  StartTrackingPointer(pointer_id, pointer_target_found);
+}
+
+void EventDispatcher::UpdateNonClientAreaForCurrentWindowOnFoundWindow(
+    const DeepestWindow& deepest_window,
+    const gfx::Point& new_location,
+    const int64_t new_display_id) {
+  event_targeter_->ProcessNextHittesetRequestFromQueue();
+
+  if (deepest_window.window == mouse_cursor_source_window_) {
+    mouse_cursor_in_non_client_area_ =
+        mouse_cursor_source_window_ ? deepest_window.in_non_client_area : false;
+  }
+  UpdateMousePointerLocation(new_location, new_display_id);
+  delegate_->UpdateNativeCursorFromDispatcher();
+}
+
+void EventDispatcher::UpdateCursorProviderByLastKnownLocationOnFoundWindow(
+    const DeepestWindow& deepest_window,
+    const gfx::Point& new_location,
+    const int64_t new_display_id) {
+  event_targeter_->ProcessNextHittesetRequestFromQueue();
+  UpdateCursorProviderByLastKnownLocationWithWindow(
+      deepest_window, new_location, new_display_id);
+}
+
+void EventDispatcher::UpdateCursorProviderByLastKnownLocationWithWindow(
+    const DeepestWindow& deepest_window,
+    const gfx::Point& new_location,
+    const int64_t new_display_id) {
+  SetMouseCursorSourceWindow(deepest_window.window);
+  if (mouse_cursor_source_window_) {
+    mouse_cursor_in_non_client_area_ = deepest_window.in_non_client_area;
+  } else {
+    SetMouseCursorSourceWindow(delegate_->GetRootWindowContaining(
+        &mouse_pointer_last_location_, &mouse_pointer_display_id_));
+    mouse_cursor_in_non_client_area_ = true;
+  }
+  UpdateMousePointerLocation(new_location, new_display_id);
+  delegate_->UpdateNativeCursorFromDispatcher();
 }
 
 bool EventDispatcher::AreAnyPointersDown() const {