Keep root_location to be in pixels and display coordinates in WS.

services/ui/ws/window_manager_state.cc

Index: services/ui/ws/window_manager_state.cc
diff --git a/services/ui/ws/window_manager_state.cc b/services/ui/ws/window_manager_state.cc
index 6657f1500c52b9d070a88b1ef07e96d2023a9ba1..6406590c5b73d68a265d896f02452570293f7bb8 100644
--- a/services/ui/ws/window_manager_state.cc
+++ b/services/ui/ws/window_manager_state.cc
@@ -22,6 +22,7 @@
 #include "services/ui/ws/window_server.h"
 #include "services/ui/ws/window_tree.h"
 #include "ui/events/event.h"
+#include "ui/gfx/geometry/dip_util.h"
 
 namespace ui {
 namespace ws {
@@ -273,10 +274,12 @@ bool WindowManagerState::IsActive() const {
   return window_server()->user_id_tracker()->active_id() == user_id();
 }
 
-void WindowManagerState::Activate(const gfx::Point& mouse_location_on_screen) {
+void WindowManagerState::Activate(const gfx::Point& mouse_location_on_display,
+                                  const int64_t display_id) {
   SetAllRootWindowsVisible(true);
   event_dispatcher_.Reset();
-  event_dispatcher_.SetMousePointerScreenLocation(mouse_location_on_screen);
+  event_dispatcher_.SetMousePointerDisplayLocationAndId(
+      mouse_location_on_display, display_id);
 }
 
 void WindowManagerState::Deactivate() {
@@ -323,7 +326,8 @@ void WindowManagerState::OnAcceleratorAck(
       details->event->AsKeyEvent()->SetProperties(properties);
     event_processing_display_id_ = details->display_id;
     event_dispatcher_.ProcessEvent(
-        *details->event, EventDispatcher::AcceleratorMatchPhase::POST_ONLY);
+        *details->event, event_processing_display_id_,
+        EventDispatcher::AcceleratorMatchPhase::POST_ONLY);
   } else {
     // We're not going to process the event any further, notify event observers.
     // We don't do this first to ensure we don't send an event twice to clients.
@@ -419,7 +423,7 @@ void WindowManagerState::ProcessEventImpl(const ui::Event& event,
   // Debug accelerators are always checked and don't interfere with processing.
   ProcessDebugAccelerator(event);
   event_processing_display_id_ = display_id;
-  event_dispatcher_.ProcessEvent(event,
+  event_dispatcher_.ProcessEvent(event, event_processing_display_id_,
                                  EventDispatcher::AcceleratorMatchPhase::ANY);
 }
 
@@ -540,6 +544,19 @@ void WindowManagerState::ScheduleInputEventTimeout(WindowTree* tree,
   in_flight_event_details_ = std::move(details);
 }
 
+bool WindowManagerState::ConvertPointToScreen(gfx::Point* point,
+                                              const int64_t display_id) {
+  Display* display = display_manager()->GetDisplayById(display_id);
+  if (display) {
+    const display::Display& originated_display = display->GetDisplay();
+    *point = gfx::ConvertPointToDIP(originated_display.device_scale_factor(),
+                                    *point);
+    *point += originated_display.bounds().origin().OffsetFromOrigin();
+    return true;
+  }
+  return false;
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // EventDispatcherDelegate:
 
@@ -611,11 +628,15 @@ void WindowManagerState::OnCaptureChanged(ServerWindow* new_capture,
   window_server()->ProcessCaptureChanged(new_capture, old_capture);
 }
 
-void WindowManagerState::OnMouseCursorLocationChanged(const gfx::Point& point) {
+void WindowManagerState::OnMouseCursorLocationChanged(
+    const gfx::Point& point_in_display,
+    const int64_t display_id) {
+  gfx::Point point_in_screen(point_in_display);
+  ConvertPointToScreen(&point_in_screen, display_id);
   window_server()
       ->display_manager()
       ->GetCursorLocationManager(user_id())
-      ->OnMouseCursorLocationChanged(point);
+      ->OnMouseCursorLocationChanged(point_in_screen);

[sky, 2017/05/17 16:53:42]

Won't this lead to rounding errors? By that I mean should this be in float? Is
the client code expecting DIPs right now? Please also update the documentation
for CursorLocationManager to say it's in dips.

[sky, 2017/05/17 19:02:07]

Don't worry about the rounding errors for now. We can deal with that later.

[riajiang, 2017/05/19 20:45:05]

This won't introduce new rounding error? We were also converting from display
coordinates to screen coordinates before for the event root_location which is
where mouse cursor location comes from?

CursorLocationManager is expecting screen-coord and DIP (updated comment)
because this OnMouseCursorLocationChanged was taking in mouse cursor location in
screen-coord and DIP. 

[sky, 2017/05/19 22:12:19]

Before we were setting the location to pixels, so no rounding errors. Once you
convert to DIPs as ints then it is impossible for client code that sees the dip
location to convert back to pixels and keep the precision. But this can be dealt
with separately.

 }
 
 void WindowManagerState::DispatchInputEventToWindow(ServerWindow* target,
@@ -668,33 +689,43 @@ ClientSpecificId WindowManagerState::GetEventTargetClientId(
 }
 
 ServerWindow* WindowManagerState::GetRootWindowContaining(
-    gfx::Point* location) {
+    gfx::Point* location_in_display,
+    int64_t* display_id) {

[sky, 2017/05/17 16:53:42]

I'm still confused about this. I thought all events originate with a display_id
and this code shouldn't need to figure it out?

[riajiang, 2017/05/19 20:45:05]

For most events the display id for the |target_display_root| we found would be
the same as the |display_id| param. But if it's a drag-and-drop, the mouse move
event would be relative to the first display even when we have moved the mouse
to the second display. In that case, after this function call,
|location_in_display| and |display_id| would be updated to be relative to the
second display. Am I misunderstanding sth?

[sky, 2017/05/19 22:12:19]

Are you *sure* about this? Again, I believe events always originate from the
display the cursor is on. Even during drag and drop. My understanding is that
the only way for the cursor onto a different display is via warp. Have you tried
this on an actual device? If running on the desktop produces different behavior
I would say we should make the desktop give the same behavior so we don't need
to special case it. Sadrul knows more than I in this area, so I encourage you to
talk with him about how to handle this.

   if (window_manager_display_roots_.empty())
     return nullptr;
 
-  // TODO(riajiang): This is broken for HDPI because it mixes PPs and DIPs. See
-  // http://crbug.com/701036 for details.
   WindowManagerDisplayRoot* target_display_root = nullptr;
-  for (auto& display_root_ptr : window_manager_display_roots_) {
-    if (display_root_ptr->display()->GetDisplay().bounds().Contains(
-            *location)) {
-      target_display_root = display_root_ptr.get();
-      break;
+  gfx::Point location_in_screen(*location_in_display);
+  if (ConvertPointToScreen(&location_in_screen, *display_id)) {
+    for (auto& display_root_ptr : window_manager_display_roots_) {
+      if (display_root_ptr->display()->GetDisplay().bounds().Contains(
+              location_in_screen)) {
+        target_display_root = display_root_ptr.get();
+        break;
+      }
     }
   }
 
   // TODO(kylechar): Better handle locations outside the window. Overlapping X11
   // windows, dragging and touch sensors need to be handled properly.
   if (!target_display_root) {
-    DVLOG(1) << "Invalid event location " << location->ToString();
+    DVLOG(1) << "Invalid event location " << location_in_display->ToString()
+             << " / display id " << *display_id;
     target_display_root = window_manager_display_roots_.begin()->get();
   }
 
-  // Translate the location to be relative to the display instead of relative
-  // to the screen space.
-  gfx::Point origin =
-      target_display_root->display()->GetDisplay().bounds().origin();
-  *location -= origin.OffsetFromOrigin();
+  // Update |location_in_display| and |display_id| if the target display is
+  // different from the originated display, e.g. drag-and-drop.
+  if (*display_id != target_display_root->display()->GetId()) {
+    gfx::Point origin =
+        target_display_root->display()->GetDisplay().bounds().origin();
+    *location_in_display = location_in_screen - origin.OffsetFromOrigin();
+    *location_in_display = gfx::ConvertPointToPixel(
+        target_display_root->display()->GetDisplay().device_scale_factor(),
+        *location_in_display);
+    *display_id = target_display_root->display()->GetId();
+  }
+
   return target_display_root->GetClientVisibleRoot();
 }