exo: Fix cursor scale when crossing displays

components/exo/pointer.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/exo/pointer.h"
 
 #include "ash/public/cpp/shell_window_ids.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/output/copy_output_result.h"
 #include "components/exo/pointer_delegate.h"
@@ skipping 110 matching lines @@
     cursor_changed = true;
   }
 
   // Early out if cursor did not change.
   if (!cursor_changed)
     return;
 
   // 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);
   }
 }
 
 gfx::NativeCursor Pointer::GetCursor() {
   if (focus_)
     if (auto* root_window = focus_->window()->GetRootWindow())
       if (auto* cursor_client = aura::client::GetCursorClient(root_window))
         return cursor_client->GetCursor();
 
   return ui::kCursorNull;
@@ skipping 95 matching lines @@
 void Pointer::OnScrollEvent(ui::ScrollEvent* event) {
   OnMouseEvent(event);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // WMHelper::CursorObserver overrides:
 
 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) {
   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/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);
+  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // SurfaceDelegate overrides:
 
 void Pointer::OnSurfaceCommit() {
   surface_->CheckIfSurfaceHierarchyNeedsCommitToNewSurfaces();
   surface_->CommitSurfaceHierarchy();
 
   // Capture new cursor to reflect result of commit.
   if (focus_)
-    CaptureCursor();
+    CaptureCursor(false);
 }
 
 bool Pointer::IsSurfaceSynchronized() const {
   // A pointer surface is always desynchronized.
   return false;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // SurfaceObserver overrides:
 
@@ skipping 11 matching lines @@
 
 Surface* Pointer::GetEffectiveTargetForEvent(ui::Event* event) const {
   Surface* target =
       Surface::AsSurface(static_cast<aura::Window*>(event->target()));
   if (!target)
     return nullptr;
 
   return delegate_->CanAcceptPointerEventsForSurface(target) ? target : nullptr;
 }
 
-void Pointer::CaptureCursor() {
+void Pointer::CaptureCursor(bool force) {
   DCHECK(surface_);
   DCHECK(focus_);
 
   float scale = cursor_scale_ * display_scale_;
   float layer_scale = scale / 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_, 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;
 
   gfx::NativeCursor cursor = ui::kCursorNull;
   if (!result->IsEmpty()) {
     DCHECK(result->HasBitmap());
     std::unique_ptr<SkBitmap> bitmap = result->TakeBitmap();
 
     ui::PlatformCursor platform_cursor;
 #if defined(USE_OZONE)
     // TODO(reveman): Add interface for creating cursors from GpuMemoryBuffers
     // and use that here instead of the current bitmap API. crbug.com/686600
     platform_cursor = ui::CursorFactoryOzone::GetInstance()->CreateImageCursor(
         *bitmap.get(), hotspot);
 #elif defined(USE_X11)
     XcursorImage* image = ui::SkBitmapToXcursorImage(bitmap.get(), hotspot);
     platform_cursor = ui::CreateReffedCustomXCursor(image);
 #endif
     cursor = ui::kCursorCustom;
     cursor.SetPlatformCursor(platform_cursor);
 #if defined(USE_OZONE)
     ui::CursorFactoryOzone::GetInstance()->UnrefImageCursor(platform_cursor);
 #elif defined(USE_X11)
     ui::UnrefCustomXCursor(platform_cursor);
 #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();
   if (!root_window)
     return;
 
   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