exo: Fix cursor scale when crossing displays

components/exo/pointer.cc

Index: components/exo/pointer.cc
diff --git a/components/exo/pointer.cc b/components/exo/pointer.cc
index 904a4bc231317355a957f790109f0cebaee24eba..26934023850aa7e674c3396e34717c185f2a9a7b 100644
--- a/components/exo/pointer.cc
+++ b/components/exo/pointer.cc
@@ -128,10 +128,10 @@ 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();
-    UpdateCursor(ui::kCursorNone);
+    UpdateCursor(ui::kCursorNone, false);
   }
 }
 
@@ -247,7 +247,7 @@ void Pointer::OnScrollEvent(ui::ScrollEvent* event) {
 void Pointer::OnCursorSetChanged(ui::CursorSetType cursor_set) {
   cursor_scale_ = GetCursorScale(cursor_set);
   if (focus_ && surface_)
-    CaptureCursor();
+    CaptureCursor(false);
 }
 
 void Pointer::OnCursorDisplayChanging(const display::Display& display) {
@@ -265,6 +265,14 @@ void Pointer::OnCursorDisplayChanging(const display::Display& display) {
   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/04/26 23:41:26]

why shouldn't we keep the old cursor until a new one has arrived?

[Dominik Laskowski, 2017/04/27 20:27:19]

Because the old bitmap may have an incorrect scale or rotation for the new
display.

[reveman, 2017/04/27 21:44:14]

Why is it better to transition to no cursor while waiting for the new one to
arrive? Seems better to just do one transition, from old to new size.

[Dominik Laskowski, 2017/04/27 22:22:17]

Because the incorrect transform is noticeable, e.g. huge cursor for a frame or
two when crossing from high-DPI to low-DPI displays.

+    cursor_capture_weak_ptr_factory_.InvalidateWeakPtrs();
+    UpdateCursor(ui::kCursorNone, true);
+    // Force the cursor to be updated even if locked.

[reveman, 2017/04/26 23:41:26]

Is "force" only needed for the case when we drag an arc window across display
boundaries? It's unfortunate that the current multi-display logic requires these
kind of special cases. Would it be possible to generalize this a bit. What if we
allow the cursor to always be changed if the currently locked cursor is the
current cursor for this pointer?  

[Dominik Laskowski, 2017/04/27 20:27:20]

Right, and it's only needed because the cursor is locked. In practice, the
client doesn't change the cursor while dragging, so we could remove the lock
instead. WDYT?

[reveman, 2017/04/27 21:44:14]

What does removing the lock mean?

[Dominik Laskowski, 2017/04/27 22:22:17]

Removing the LockCursor/UnlockCursor calls, i.e. allowing the client to update
the cursor during dragging.

+    CaptureCursor(true);
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -276,7 +284,7 @@ void Pointer::OnSurfaceCommit() {
 
   // Capture new cursor to reflect result of commit.
   if (focus_)
-    CaptureCursor();
+    CaptureCursor(false);
 }
 
 bool Pointer::IsSurfaceSynchronized() const {
@@ -308,7 +316,7 @@ Surface* Pointer::GetEffectiveTargetForEvent(ui::Event* event) const {
   return delegate_->CanAcceptPointerEventsForSurface(target) ? target : nullptr;
 }
 
-void Pointer::CaptureCursor() {
+void Pointer::CaptureCursor(bool force) {
   DCHECK(surface_);
   DCHECK(focus_);
 
@@ -323,13 +331,18 @@ void Pointer::CaptureCursor() {
       cc::CopyOutputRequest::CreateBitmapRequest(
           base::Bind(&Pointer::OnCursorCaptured,
                      cursor_capture_weak_ptr_factory_.GetWeakPtr(),
-                     gfx::ScaleToFlooredPoint(hotspot_, scale)));
+                     gfx::ScaleToFlooredPoint(hotspot_, scale), force));
 
   request->set_source(cursor_capture_source_id_);
   cursor_->layer()->RequestCopyOfOutput(std::move(request));
+
+  // Prevent subsequent requests in the same frame from aborting this capture.

[reveman, 2017/04/26 23:41:26]

why is this needed and correct?

[Dominik Laskowski, 2017/04/27 20:27:20]

It's a hack to ensure that the lock is bypassed. RequestCopyOfOutput aborts
pending requests with the same source ID, so a forced capture may be aborted if
the client happens to commit in the same frame.

[reveman, 2017/04/27 21:44:14]

Ok, let's figure out how we can avoid this "force" thing so we don't need this
kind of hack. I'm not even sure that showing the perfect cursor during drag is
that important unless this multi-display mechanism with client side drags is
what we plan on using long term.

[Dominik Laskowski, 2017/04/27 22:22:17]

Yeah, this is a non-issue with server decorations. How about we keep it simple
and remove cursor locking for now?

+  if (force)
+    cursor_capture_source_id_ = base::UnguessableToken::Create();
 }
 
 void Pointer::OnCursorCaptured(const gfx::Point& hotspot,
+                               bool force,
                                std::unique_ptr<cc::CopyOutputResult> result) {
   if (!focus_)
     return;
@@ -358,10 +371,10 @@ void Pointer::OnCursorCaptured(const gfx::Point& hotspot,
 #endif
   }
 
-  UpdateCursor(cursor);
+  UpdateCursor(cursor, force);
 }
 
-void Pointer::UpdateCursor(gfx::NativeCursor cursor) {
+void Pointer::UpdateCursor(gfx::NativeCursor cursor, bool force) {
   DCHECK(focus_);
 
   aura::Window* root_window = focus_->window()->GetRootWindow();
@@ -370,8 +383,13 @@ void Pointer::UpdateCursor(gfx::NativeCursor cursor) {
 
   aura::client::CursorClient* cursor_client =
       aura::client::GetCursorClient(root_window);
-  if (cursor_client)
-    cursor_client->SetCursor(cursor);
+  if (!cursor_client)
+    return;
+
+  cursor_client->SetCursor(cursor);

[reveman, 2017/04/26 23:41:26]

can we avoid having this line? "force || cursor_client->IsCursorLocked()" below
instead?

[Dominik Laskowski, 2017/04/27 20:27:20]

The line below updates the current cursor, and this line updates the future
unlocked cursor.

[reveman, 2017/04/27 21:44:14]

That's confusing and deserves a comment at least.

+
+  if (force && cursor_client->IsCursorLocked())
+    WMHelper::GetInstance()->SetCursor(cursor);
 }
 
 }  // namespace exo