ozone/wayland: Ensure server-side cursor shapes are always used on lacros [chromium/src : main]
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane, Antonio Gomes.
Nick Yamane uploaded patch set #3 to this change.
ozone/wayland: Ensure server-side cursor shapes are always used on lacros
Server-side cursor shapes support was first introduced at [1]. Though,
in the wl_pointer::enter event handler, it is not used, which leads to
empty cursor occasionally.
R=toni...@igalia.com
Bug: 1199408
Change-Id: I935f120dffccd9c8e4e5d962fb86290e914e00a4
---
M ui/ozone/platform/wayland/host/wayland_window.cc
1 file changed, 5 insertions(+), 17 deletions(-)
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen, Antonio Gomes.
Patch set 3:Auto-Submit +1
2 comments:
Patchset:
nick, for completeness, pls add the CL that introduced the regression to the commit message, and Cc […]
I am not sure, but my guess is that it's "always" been there, since zcr_cursor_shapes support was first introduced. So, would this one be the CL?
https://chromium-review.googlesource.com/c/chromium/src/+/2516972
Patchset:
+jamescook, the original author of server-side cursors support in ozone/wayland.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen, Antonio Gomes.
Nick Yamane removed a vote from this change.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Antonio Gomes (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
Nick Yamane has uploaded this change for review.
ozone/wayland: Ensure server-side cursor shapes are always used on lacros
Server-side cursor shapes support was first introduced at [1]. Though,
in the wl_pointer::enter event handler, it is not used, which leads to
empty cursor occasionally.
This CL fixes it by re-using the SetCursor method, which fully
implements cursor handling, including server-side ones.
R=toni...@igalia.com
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2516972
Bug: 1199408
Change-Id: I935f120dffccd9c8e4e5d962fb86290e914e00a4
---
M ui/ozone/platform/wayland/host/wayland_window.cc
1 file changed, 5 insertions(+), 17 deletions(-)
diff --git a/ui/ozone/platform/wayland/host/wayland_window.cc b/ui/ozone/platform/wayland/host/wayland_window.cc
index 391f3b6..35df5fb 100644
--- a/ui/ozone/platform/wayland/host/wayland_window.cc
+++ b/ui/ozone/platform/wayland/host/wayland_window.cc
@@ -160,23 +160,11 @@
void WaylandWindow::SetPointerFocus(bool focus) {
has_pointer_focus_ = focus;
- // Whenever the window gets the pointer focus back, we must reinitialize the
- // cursor. Otherwise, it is invalidated whenever the pointer leaves the
- // surface and is not restored by the Wayland compositor.
- if (has_pointer_focus_ && bitmap_ && bitmap_->type() != CursorType::kNone) {
- // Check for theme-provided cursor.
- if (bitmap_->platform_data()) {
- connection_->SetPlatformCursor(
- reinterpret_cast<wl_cursor*>(bitmap_->platform_data()),
- buffer_scale());
- } else {
- // Translate physical pixels to DIPs.
- gfx::Point hotspot_in_dips =
- gfx::ScaleToRoundedPoint(bitmap_->hotspot(), 1.0f / ui_scale_);
- connection_->SetCursorBitmap(bitmap_->bitmaps(), hotspot_in_dips,
- buffer_scale());
- }
- }
+ // Whenever the window gets the pointer focus back, the cursor shape must be
+ // updated. Otherwise, it is invalidated upon wl_pointer::leave and is not
+ // restored by the Wayland compositor.
+ if (has_pointer_focus_)
+ SetCursor(std::move(bitmap_));
}
bool WaylandWindow::StartDrag(const ui::OSExchangeData& data,
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Antonio Gomes (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
Patch set 5:Code-Review +1
1 comment:
Patchset:
I am not sure, but my guess is that it's "always" been there, since zcr_cursor_shapes support was fi […]
Good point. I have not seen the bug until recently. Lets if james has an idea.
/cc hferreiro, who worked on cursors a lot recently.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
Nick Yamane removed a vote from this change.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen.
1 comment:
File ui/ozone/platform/wayland/host/wayland_window.cc:
is std::move appropriate here? That implies that bitmap_ has to be regenerated before each time this […]
Good point.
I used move here as a way to bypass the conditional in SetCursor (line 304), it is equivalent to, for example, SetCursor(std::exchange(bitmap_, nullptr)). And I agree that it's a bit unusual/counter-intutive. Alternatively, the code the uncoditionally set cursor shape could be extracted from SetCursor into a separate function, say SetCursorShape() and call it from here. PLMK what you guys prefer.
>That implies that bitmap_ has to be regenerated before each time this line of code is called.
bitmap is scoped_refptr, so move should be cheap. not sure if that's what you meant with "regenerated".
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Erik Chen (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
1 comment:
File ui/ozone/platform/wayland/host/wayland_window.cc:
Good point. […]
I don't undersatnd this code very well, but if you're trying to bypass the check: "if (bitmap_ == platform_cursor)"
Then I would suggest adding a new parameter: bool force_update
if (bitmap_ == platform_cursor && !force_update)
return;
...That being said -- I'm not an owner of this class and I'm not familiar with this code, so I defer to actual owners.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen.
1 comment:
File ui/ozone/platform/wayland/host/wayland_window.cc:
I don't undersatnd this code very well, but if you're trying to bypass the check: "if (bitmap_ == […]
SetCursor() can't be changed cause it's part of PlatformWindow interface. That's why I mentioned extracting into a separate function.
Done it in the latest PS. tonikitoo@ PTAL.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Erik Chen (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
Patch set 6:Code-Review +1Commit-Queue +2
1 comment:
Patchset:
Looks much better, thanks. Please wait for owner re-review as appropriate -- I defer to your judgement.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
Nick Yamane removed a vote from this change.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Antonio Gomes (Gerrit)
Attention is currently required from: Erik Chen, Nick Yamane.
Patch set 6:Code-Review +1
2 comments:
Commit Message:
This CL fixes it by re-using the SetCursor method, which fully
implements cursor handling, including server-side ones.
maybe mentioning that the previous impl did not take `zcr-cursor-shapes` into account is relevant here.
Patchset:
Looks much better, thanks. […]
+1
it looks much better now
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen.
Nick Yamane removed Erik Chen from this change.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen.
Patch set 7:Auto-Submit +1Commit-Queue +1
1 comment:
Commit Message:
This CL fixes it by re-using the SetCursor method, which fully
implements cursor handling, including server-side ones.
maybe mentioning that the previous impl did not take `zcr-cursor-shapes` into account is relevant he […]
Done
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Erik Chen.
Patch set 7:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
ozone/wayland: Ensure server-side cursor shapes are always used on lacros
Server-side cursor shapes support was first introduced at [1]. Though,
in the wl_pointer::enter event handler, it is not used, which leads to
empty cursor occasionally.
This CL fixes it by extracting the code that deals with cursor shape
update, including zcr-cursor-shapes handling, out of SetCursor into a
new function, namely UpdateCursorShape, and call it from SetPointFocus,
if applicable.
R=toni...@igalia.com
[1] https://chromium-review.googlesource.com/c/chromium/src/+/2516972
Bug: 1199408
Change-Id: I935f120dffccd9c8e4e5d962fb86290e914e00a4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2846448
Auto-Submit: Nick Yamane <nick...@igalia.com>
Commit-Queue: Nick Yamane <nick...@igalia.com>
Reviewed-by: Erik Chen <erik...@chromium.org>
Reviewed-by: Antonio Gomes <toni...@igalia.com>
Cr-Commit-Position: refs/heads/master@{#875439}
---
M ui/ozone/platform/wayland/host/wayland_window.cc
M ui/ozone/platform/wayland/host/wayland_window.h
2 files changed, 40 insertions(+), 46 deletions(-)
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.
James Cook (Gerrit)
Patch set 8:Code-Review +1
1 comment:
Patchset:
belated LGTM, I was on vacation last Thu/Fri.
FYI - I suspect this bug has existed since I added server-side cursor support.
To view, visit change 2846448. To unsubscribe, or for help writing mail filters, visit settings.