exo: Fix multi-display hardware cursor

components/exo/pointer.cc

Index: components/exo/pointer.cc
diff --git a/components/exo/pointer.cc b/components/exo/pointer.cc
index 092501a46248c805c0403528c72492abcd7e9d73..be71559b947628b6b2a11c43e8cc208da1776f1b 100644
--- a/components/exo/pointer.cc
+++ b/components/exo/pointer.cc
@@ -19,7 +19,7 @@
 #include "ui/display/screen.h"
 #include "ui/events/event.h"
 #include "ui/gfx/geometry/vector2d_conversions.h"
-#include "ui/gfx/transform_util.h"
+#include "ui/gfx/transform.h"
 
 #if defined(USE_OZONE)
 #include "ui/ozone/public/cursor_factory_ozone.h"
@@ -32,7 +32,12 @@
 namespace exo {
 namespace {
 
-const float kLargeCursorScale = 2.8f;
+// TODO: Some accessibility features, including large cursors, disable hardware

[reveman, 2017/03/30 07:47:53]

nit: TODO(name) and please add a bug number

[Dominik Laskowski, 2017/04/05 17:59:25]

Done. TODO(oshima) to be consistent with:

https://chromium.googlesource.com/chromium/src/+/master/ash/wm/ash_native_cur...

+// cursors. Ash does not support compositing for custom cursors, so it replaces
+// them with the default cursor. As a result, this scale has no effect for now.
+float GetCursorScale(ui::CursorSetType cursor_set) {
+  return cursor_set == ui::CURSOR_SET_LARGE ? 2.8f : 1.0f;
+}
 
 // Synthesized events typically lack floating point precision so to avoid
 // generating mouse event jitter we consider the location of these events
@@ -51,12 +56,19 @@ bool SameLocation(const ui::LocatedEvent* event, const gfx::PointF& location) {
 
 Pointer::Pointer(PointerDelegate* delegate)
     : delegate_(delegate),
-      cursor_(ui::kCursorNull),
+      cursor_(new aura::Window(nullptr)),
       cursor_capture_source_id_(base::UnguessableToken::Create()),
       cursor_capture_weak_ptr_factory_(this) {
   auto* helper = WMHelper::GetInstance();
   helper->AddPreTargetHandler(this);
   helper->AddCursorObserver(this);
+
+  cursor_scale_ = GetCursorScale(helper->GetCursorSet());
+
+  cursor_->SetName("ExoCursor");
+  cursor_->Init(ui::LAYER_NOT_DRAWN);
+  cursor_->set_owned_by_parent(false);
+  OnCursorDisplayChanging(display::Screen::GetScreen()->GetPrimaryDisplay());
 }
 
 Pointer::~Pointer() {
@@ -88,10 +100,7 @@ void Pointer::SetCursor(Surface* surface, const gfx::Point& hotspot) {
       return;
     }
     if (surface_) {
-      surface_->window()->SetTransform(gfx::Transform());
-      WMHelper::GetInstance()
-          ->GetContainer(ash::kShellWindowId_MouseCursorContainer)
-          ->RemoveChild(surface_->window());
+      cursor_->RemoveChild(surface_->window());
       surface_->SetSurfaceDelegate(nullptr);
       surface_->RemoveSurfaceObserver(this);
     }
@@ -99,12 +108,8 @@ void Pointer::SetCursor(Surface* surface, const gfx::Point& hotspot) {
     if (surface_) {
       surface_->SetSurfaceDelegate(this);
       surface_->AddSurfaceObserver(this);
-      // Note: Surface window needs to be added to the tree so we can take a
-      // snapshot. Where in the tree is not important but we might as well use
-      // the cursor container.
-      WMHelper::GetInstance()
-          ->GetContainer(ash::kShellWindowId_MouseCursorContainer)
-          ->AddChild(surface_->window());
+      cursor_->AddChild(surface_->window());
+      surface_->window()->Show();
     }
     cursor_changed = true;
   }
@@ -122,16 +127,20 @@ void Pointer::SetCursor(Surface* surface, const gfx::Point& hotspot) {
   // If |surface_| is set then asynchronously capture a snapshot of cursor,
   // otherwise cancel pending capture and immediately set the cursor to "none".
   if (surface_) {
-    CaptureCursor();
+    CaptureCursor(false);
   } else {
     cursor_capture_weak_ptr_factory_.InvalidateWeakPtrs();
-    cursor_ = ui::kCursorNone;
-    UpdateCursor();
+    UpdateCursor(ui::kCursorNone, false);
   }
 }
 
 gfx::NativeCursor Pointer::GetCursor() {
-  return cursor_;

[reveman, 2017/03/30 07:47:53]

why does all this need to change? I think the previous logic was easy to
understand.

[Dominik Laskowski, 2017/04/05 17:59:25]

Keeping track of the latest cursor is unnecessary, i.e. CursorClient::GetCursor
does that already.

+  if (!focus_)
+    return ui::kCursorNull;

[reveman, 2017/03/30 07:47:53]

I think returning null cursor in this case is incorrect. It's critical that the
cursor is not changing when entering a new window until the client has had a
chance to replace it.

[Dominik Laskowski, 2017/04/05 17:59:25]

This is equivalent to the old code. Also, the cursor is captured on mouse enter
below, so the null cursor (i.e. the default pointer) would only appear while the
capture is pending.

+
+  aura::client::CursorClient* cursor_client =
+      aura::client::GetCursorClient(focus_->window()->GetRootWindow());
+  return cursor_client ? cursor_client->GetCursor() : ui::kCursorNull;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -151,7 +160,6 @@ void Pointer::OnMouseEvent(ui::MouseEvent* event) {
       // response to each OnPointerEnter() call.
       focus_->UnregisterCursorProvider(this);
       focus_ = nullptr;
-      cursor_ = ui::kCursorNull;
       cursor_capture_weak_ptr_factory_.InvalidateWeakPtrs();
     }
     // Second generate an enter event if focus moved to a new target.
@@ -162,6 +170,9 @@ void Pointer::OnMouseEvent(ui::MouseEvent* event) {
       focus_ = target;
       focus_->AddSurfaceObserver(this);
       focus_->RegisterCursorProvider(this);
+
+      if (surface_)

[reveman, 2017/03/30 07:47:53]

this is incorrect I think. the client needs to provide a new cursor surface for
each enter event.

[Dominik Laskowski, 2017/04/05 17:59:25]

The client does not do that currently. Is that required by the spec?

+        CaptureCursor(false);
     }
     delegate_->OnPointerFrame();
   }
@@ -226,8 +237,6 @@ void Pointer::OnMouseEvent(ui::MouseEvent* event) {
       NOTREACHED();
       break;
   }
-
-  UpdateCursorScale();
 }
 
 void Pointer::OnScrollEvent(ui::ScrollEvent* event) {
@@ -238,8 +247,34 @@ void Pointer::OnScrollEvent(ui::ScrollEvent* event) {
 // WMHelper::CursorObserver overrides:
 
 void Pointer::OnCursorSetChanged(ui::CursorSetType cursor_set) {
-  if (focus_)
-    UpdateCursorScale();
+  cursor_scale_ = GetCursorScale(cursor_set);
+  if (focus_ && surface_)
+    CaptureCursor(false);
+}
+
+void Pointer::OnCursorDisplayChanging(const display::Display& display) {
+  WMHelper* helper = WMHelper::GetInstance();
+  aura::Window* container = helper->GetContainer(
+      display.id(), ash::kShellWindowId_MouseCursorContainer);
+
+  // Reparent the cursor to the root window where the mouse is located.
+  if (container->GetRootWindow() != cursor_->GetRootWindow()) {
+    if (cursor_->parent())
+      cursor_->parent()->RemoveChild(cursor_.get());
+    container->AddChild(cursor_.get());
+  }
+
+  auto info = helper->GetDisplayInfo(display.id());
+  display_scale_ = info.GetEffectiveUIScale() * info.device_scale_factor();
+  device_scale_factor_ = display.device_scale_factor();
+
+  if (focus_ && surface_) {
+    // Capture is asynchronous, so avoid rendering old cursor in the meantime.

[reveman, 2017/03/30 07:47:53]

I don't understand why we need to do all this. This type of special case where
we force an update makes me worried about race conditions where we incorrectly
cancel the capture of a cursor frame. It's important that cursor changes can be
pipelined properly or animated cursor will not work correctly.

[Dominik Laskowski, 2017/04/05 17:59:25]

This special case is needed when dragging across displays. While dragging, the
cursor is locked, so CursorClient::SetCursor calls are collapsed into a single
call at the end of the drag. The cursor cannot be animated in that case. When
crossing displays, we need to force a capture to update the scale, and hide the
old cursor while the capture is pending.

+    cursor_capture_weak_ptr_factory_.InvalidateWeakPtrs();
+    UpdateCursor(ui::kCursorNone, true);
+    // Force the cursor to be updated even if locked.
+    CaptureCursor(true);
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -251,7 +286,7 @@ void Pointer::OnSurfaceCommit() {
 
   // Capture new cursor to reflect result of commit.
   if (focus_)
-    CaptureCursor();
+    CaptureCursor(false);
 }
 
 bool Pointer::IsSurfaceSynchronized() const {
@@ -283,72 +318,38 @@ Surface* Pointer::GetEffectiveTargetForEvent(ui::Event* event) const {
   return delegate_->CanAcceptPointerEventsForSurface(target) ? target : nullptr;
 }
 
-void Pointer::UpdateCursorScale() {
-  DCHECK(focus_);
-
-  display::Screen* screen = display::Screen::GetScreen();
-  WMHelper* helper = WMHelper::GetInstance();
-
-  // Update cursor scale if the effective UI scale has changed.
-  display::Display display = screen->GetDisplayNearestWindow(focus_->window());
-  float scale = helper->GetDisplayInfo(display.id()).GetEffectiveUIScale();
-
-  if (display::Display::HasInternalDisplay()) {
-    float primary_device_scale_factor =
-        screen->GetPrimaryDisplay().device_scale_factor();
-    // The size of the cursor surface is the quotient of its physical size and
-    // the DSF of the primary display. The physical size is proportional to the
-    // DSF of the internal display. For external displays (and the internal
-    // display when secondary to a display with a different DSF), scale the
-    // cursor so its physical size matches with the single display case.
-    if (!display.IsInternal() ||
-        display.device_scale_factor() != primary_device_scale_factor) {
-      scale *= primary_device_scale_factor /
-               helper->GetDisplayInfo(display::Display::InternalDisplayId())
-                   .device_scale_factor();
-    }
-  }
-
-  if (helper->GetCursorSet() == ui::CURSOR_SET_LARGE)
-    scale *= kLargeCursorScale;
-
-  if (scale != cursor_scale_) {
-    cursor_scale_ = scale;
-    if (surface_)
-      CaptureCursor();
-  }
-}
-
-void Pointer::CaptureCursor() {
+void Pointer::CaptureCursor(bool force) {
   DCHECK(surface_);
   DCHECK(focus_);
 
-  // Set UI scale before submitting capture request.
-  surface_->window()->layer()->SetTransform(
-      gfx::GetScaleTransform(gfx::Point(), cursor_scale_));
+  float scale = cursor_scale_ * display_scale_;
+  float layer_scale = scale / device_scale_factor_;
 
-  float primary_device_scale_factor =
-      display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();
+  gfx::Transform transform;
+  transform.Scale(layer_scale, layer_scale);
+  surface_->window()->SetTransform(transform);
 
   std::unique_ptr<cc::CopyOutputRequest> request =
       cc::CopyOutputRequest::CreateBitmapRequest(
           base::Bind(&Pointer::OnCursorCaptured,
                      cursor_capture_weak_ptr_factory_.GetWeakPtr(),
-                     gfx::ScaleToFlooredPoint(
-                         hotspot_,
-                         // |hotspot_| is in surface coordinate space so apply
-                         // both device scale and UI scale.
-                         cursor_scale_ * primary_device_scale_factor)));
+                     gfx::ScaleToFlooredPoint(hotspot_, scale), force));
+
   request->set_source(cursor_capture_source_id_);
-  surface_->window()->layer()->RequestCopyOfOutput(std::move(request));
+  cursor_->layer()->RequestCopyOfOutput(std::move(request));
+
+  // Prevent subsequent requests in the same frame from aborting this capture.

[reveman, 2017/03/30 07:47:53]

why is this needed?

[Dominik Laskowski, 2017/04/05 17:59:25]

If the client happens to commit or call wl_pointer::set_cursor while the cursor
is locked, we don't want those capture requests to cancel a pending request for
a forced capture. See above for the special case where |force| is true.

+  if (force)
+    cursor_capture_source_id_ = base::UnguessableToken::Create();
 }
 
-void Pointer::OnCursorCaptured(const gfx::Point& hotspot,
+void Pointer::OnCursorCaptured(gfx::Point hotspot,
+                               bool force,
                                std::unique_ptr<cc::CopyOutputResult> result) {
   if (!focus_)
     return;
 
-  cursor_ = ui::kCursorNone;
+  gfx::NativeCursor cursor = ui::kCursorNull;
   if (!result->IsEmpty()) {
     DCHECK(result->HasBitmap());
     std::unique_ptr<SkBitmap> bitmap = result->TakeBitmap();
@@ -363,8 +364,8 @@ void Pointer::OnCursorCaptured(const gfx::Point& hotspot,
     XcursorImage* image = ui::SkBitmapToXcursorImage(bitmap.get(), hotspot);
     platform_cursor = ui::CreateReffedCustomXCursor(image);
 #endif
-    cursor_ = ui::kCursorCustom;
-    cursor_.SetPlatformCursor(platform_cursor);
+    cursor = ui::kCursorCustom;
+    cursor.SetPlatformCursor(platform_cursor);
 #if defined(USE_OZONE)
     ui::CursorFactoryOzone::GetInstance()->UnrefImageCursor(platform_cursor);
 #elif defined(USE_X11)
@@ -372,16 +373,21 @@ void Pointer::OnCursorCaptured(const gfx::Point& hotspot,
 #endif
   }
 
-  UpdateCursor();
+  UpdateCursor(cursor, force);
 }
 
-void Pointer::UpdateCursor() {
+void Pointer::UpdateCursor(gfx::NativeCursor cursor, bool force) {
   DCHECK(focus_);
 
   aura::client::CursorClient* cursor_client =
       aura::client::GetCursorClient(focus_->window()->GetRootWindow());
-  if (cursor_client)
-    cursor_client->SetCursor(cursor_);
+  if (!cursor_client)
+    return;
+
+  cursor_client->SetCursor(cursor);
+
+  if (force && cursor_client->IsCursorLocked())

[reveman, 2017/03/30 07:47:53]

what's the situation where this is currently causing a problem? can this
IsCursorLocked part be moved to a separate CL?

[Dominik Laskowski, 2017/04/05 17:59:25]

|force| being true implies that the cursor is locked, but the check is there for
completeness. Also, note that CursorClient::SetCursor is called as well to
update the state applied when the cursor is unlocked.

OK, I will split out that edge case.

+    WMHelper::GetInstance()->SetCursor(cursor);
 }
 
 }  // namespace exo