TabDragging tests: Set browser bounds using mouse emulation [chromium/src : main]
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Maksim Sisov, Scott Violet.
Max Ihlenfeldt would like Maksim Sisov and Scott Violet to review this change.
TabDragging tests: Set browser bounds using mouse emulation
On Linux ozone/wayland we can't control the browser bounds, but many
TabDragging tests expect specific bounds and will fail if the actual
bounds are different.
This CL introduces a workaround that allows us to still set the browser
bounds on this platform. The idea is to use ui_controls to emulate mouse
events that will resize and move the browser to where we want (like a
regular user would do).
Bug: 1474921
Change-Id: Ie1e8c154b3f6d078b77e9900f60e7001aa329436
---
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
M testing/buildbot/filters/ozone-linux.interactive_ui_tests_wayland.filter
M ui/base/test/ui_controls.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.h
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.cc
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.h
M ui/ozone/public/ozone_ui_controls_test_helper.cc
M ui/ozone/public/ozone_ui_controls_test_helper.h
M ui/views/test/ui_controls_factory_desktop_aura_ozone.cc
11 files changed, 256 insertions(+), 44 deletions(-)
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Maksim Sisov, Scott Violet.
1 comment:
Patchset:
Maksim, PTAL at the `//ui/ozone/platform/wayland` changes.
Scott, PTAL at the rest. It's been a while, but maybe you remember that we shortly discussed this approach via email back in February.
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Max Ihlenfeldt, Scott Violet.
Patch set 13:Code-Review +1
1 comment:
File ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc:
Patch Set #13, Line 170:
// LOG(ERROR) << "window_insets="please cleanup :)
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Attention is currently required from: Max Ihlenfeldt.
3 comments:
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:
Patch Set #13, Line 249:
void ResizeUsingMouseEmulation(Browser* browser, gfx::Rect target_bounds) {To simplify the call sites how about providing a function SetBrowserBounds(). The implementation for linux is what you have here, but for all other platforms it sets the bounds directly. This hopefully keeps the complexity in the function, not at every call sites.
Patch Set #13, Line 249:
gfx::Rectconst&
File ui/base/test/ui_controls.h:
Patch Set #13, Line 187:
void ForceUseScreenCoordinates(bool use);Would it be better to provide this via OzonePlatform::InitParams? Toggling it part way through a test seems rather risky, in so far as it could effect a bunch of other random code.
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Max Ihlenfeldt.
Max Ihlenfeldt uploaded patch set #14 to this change.
TabDragging tests: Set browser bounds using mouse emulation
On Linux ozone/wayland we can't control the browser bounds, but many
TabDragging tests expect specific bounds and will fail if the actual
bounds are different.
This CL introduces a workaround that allows us to still set the browser
bounds on this platform. The idea is to use ui_controls to emulate mouse
events that will resize and move the browser to where we want (like a
regular user would do).
Bug: 1474921
Change-Id: Ie1e8c154b3f6d078b77e9900f60e7001aa329436
---
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
M testing/buildbot/filters/ozone-linux.interactive_ui_tests_wayland.filter
M ui/base/test/ui_controls.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.h
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.cc
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.h
M ui/ozone/public/ozone_ui_controls_test_helper.cc
M ui/ozone/public/ozone_ui_controls_test_helper.h
M ui/views/test/ui_controls_factory_desktop_aura_ozone.cc
11 files changed, 254 insertions(+), 44 deletions(-)
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Scott Violet.
4 comments:
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:
Patch Set #13, Line 249:
void ResizeUsingMouseEmulation(Browser* browser, gfx::Rect target_bounds) {
To simplify the call sites how about providing a function SetBrowserBounds(). […]
Doesn't the new version of `CreateAnotherBrowserAndResize()` do what you describe? This is the only place where `ResizeUsingMouseEmulation()` is called, and I don't think we can reduce its complexity further.
We could add back the original `Resize()` method (with a `CHECK(test::CanUseSetBounds())`) and use that in `DetachToBrowserTabDragControllerTestWithTabbedSystemApp.DragAppToAppWindow`, and possibly in other future tests as well.
(A variant of `ResizeUsingMouseEmulation()` that takes two `Browser*` parameters, like the original `Resize()`, currently wouldn't be useful, because `CreateAnotherBrowserAndResize()` has to resize the first browser before creating the second one; else the second browser might occlude the first browser's bottom right corner or tab strip grab handle space.)
Patch Set #13, Line 249:
gfx::Rectconst&
Done
File ui/base/test/ui_controls.h:
Patch Set #13, Line 187:
void ForceUseScreenCoordinates(bool use);
Would it be better to provide this via OzonePlatform::InitParams? Toggling it part way through a tes […]
It might indeed be somewhat risky, but I'm not sure it's possible to avoid it. Forcing the use of screen coordinates will break all emulated mouse positioning except for the one `SendMouseMoveSync()` call in `ResizeUsingMouseEmulation()`, because we don't actually have screen coordinates available on Linux ozone/wayland.
For example, if we want to move the mouse to (100, 100) relative to a given browser window's origin, we pass the coordinates (100, 100) and the window they are relative to as parameters. Forcing the use of screen coordinates would mean that this is interpreted as (100, 100) relative to the screen origin, which almost certainly will be the wrong location.
Maybe we can still minimize the risk of this, though. For example, changing `void ForceUseScreenCoordinates(bool use)` to `void ForceUseScreenCoordinatesOnce()` and making it affect only the next `ui_controls::SendMouseMove()` / `SendMouseMoveNotifyWhenDone()` call could help with avoiding it affecting unwanted code?
File ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc:
Patch Set #13, Line 170:
// LOG(ERROR) << "window_insets="please cleanup :)
Done
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Attention is currently required from: Max Ihlenfeldt.
2 comments:
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:
Patch Set #13, Line 249:
void ResizeUsingMouseEmulation(Browser* browser, gfx::Rect target_bounds) {
Doesn't the new version of `CreateAnotherBrowserAndResize()` do what you describe? This is the only place where `ResizeUsingMouseEmulation()` is called, and I don't think we can reduce its complexity further.
We could add back the original `Resize()` method (with a `CHECK(test::CanUseSetBounds())`) and use that in `DetachToBrowserTabDragControllerTestWithTabbedSystemApp.DragAppToAppWindow`, and possibly in other future tests as well.
(A variant of `ResizeUsingMouseEmulation()` that takes two `Browser*` parameters, like the original `Resize()`, currently wouldn't be useful, because `CreateAnotherBrowserAndResize()` has to resize the first browser before creating the second one; else the second browser might occlude the first browser's bottom right corner or tab strip grab handle space.)
You are absolutely right! I misread the places that needed to call
File ui/base/test/ui_controls.h:
Patch Set #13, Line 187:
void ForceUseScreenCoordinates(bool use);
It might indeed be somewhat risky, but I'm not sure it's possible to avoid it. […]
I like the Once() idea!
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Max Ihlenfeldt.
Max Ihlenfeldt uploaded patch set #15 to this change.
TabDragging tests: Set browser bounds using mouse emulation
On Linux ozone/wayland we can't control the browser bounds, but many
TabDragging tests expect specific bounds and will fail if the actual
bounds are different.
This CL introduces a workaround that allows us to still set the browser
bounds on this platform. The idea is to use ui_controls to emulate mouse
events that will resize and move the browser to where we want (like a
regular user would do).
Bug: 1474921
Change-Id: Ie1e8c154b3f6d078b77e9900f60e7001aa329436
---
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
M testing/buildbot/filters/ozone-linux.interactive_ui_tests_wayland.filter
M ui/base/test/ui_controls.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.h
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.cc
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.h
M ui/ozone/public/ozone_ui_controls_test_helper.cc
M ui/ozone/public/ozone_ui_controls_test_helper.h
M ui/views/test/ui_controls_factory_desktop_aura_ozone.cc
11 files changed, 259 insertions(+), 43 deletions(-)
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Scott Violet.
3 comments:
Patchset:
@Maksim: could you please take another look at the changes from PS 15, as they also touch ozone/wayland code?
File chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc:
Patch Set #13, Line 249:
void ResizeUsingMouseEmulation(Browser* browser, gfx::Rect target_bounds) {
> Doesn't the new version of `CreateAnotherBrowserAndResize()` do what you describe? This is the onl […]
Acknowledged
File ui/base/test/ui_controls.h:
Patch Set #13, Line 187:
void ForceUseScreenCoordinates(bool use);
I like the Once() idea!
Done
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Max Ihlenfeldt, Scott Violet.
Patch set 15:Code-Review +1
2 comments:
Patchset:
still lgtm
File ui/ozone/platform/wayland/emulate/wayland_input_emulate.h:
Patch Set #15, Line 183:
alwaysnit: remove always
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Scott Violet (Gerrit)
Attention is currently required from: Max Ihlenfeldt.
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Attention is currently required from: Max Ihlenfeldt.
Max Ihlenfeldt uploaded patch set #16 to this change.
TabDragging tests: Set browser bounds using mouse emulation
On Linux ozone/wayland we can't control the browser bounds, but many
TabDragging tests expect specific bounds and will fail if the actual
bounds are different.
This CL introduces a workaround that allows us to still set the browser
bounds on this platform. The idea is to use ui_controls to emulate mouse
events that will resize and move the browser to where we want (like a
regular user would do).
Bug: 1474921
Change-Id: Ie1e8c154b3f6d078b77e9900f60e7001aa329436
---
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
M testing/buildbot/filters/ozone-linux.interactive_ui_tests_wayland.filter
M ui/base/test/ui_controls.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.h
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.cc
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.h
M ui/ozone/public/ozone_ui_controls_test_helper.cc
M ui/ozone/public/ozone_ui_controls_test_helper.h
M ui/views/test/ui_controls_factory_desktop_aura_ozone.cc
11 files changed, 259 insertions(+), 43 deletions(-)
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Max Ihlenfeldt (Gerrit)
Patch set 16:Commit-Queue +2
2 comments:
Patchset:
Thank you both for the review!
File ui/ozone/platform/wayland/emulate/wayland_input_emulate.h:
Patch Set #15, Line 183:
alwaysnit: remove always
Done
To view, visit change 4738688. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
15 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ui/ozone/platform/wayland/emulate/wayland_input_emulate.h
Insertions: 1, Deletions: 1.
@@ -180,7 +180,7 @@
base::RepeatingCallback<void(uint32_t)> request_processed_callback_;
- // If true, the next `EmulatePointerMotion` call will always use global screen
+ // If true, the next `EmulatePointerMotion` call will use global screen
// coordinates, i.e. send zcr_ui_controls_v1.mouse_move with the `surface`
// parameter set to NULL.
// Note: this does not affect whether `EmulatePointerMotion` uses the
```
TabDragging tests: Set browser bounds using mouse emulation
On Linux ozone/wayland we can't control the browser bounds, but many
TabDragging tests expect specific bounds and will fail if the actual
bounds are different.
This CL introduces a workaround that allows us to still set the browser
bounds on this platform. The idea is to use ui_controls to emulate mouse
events that will resize and move the browser to where we want (like a
regular user would do).
Bug: 1474921
Change-Id: Ie1e8c154b3f6d078b77e9900f60e7001aa329436
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4738688
Reviewed-by: Maksim Sisov <msi...@igalia.com>
Reviewed-by: Scott Violet <s...@chromium.org>
Commit-Queue: Max Ihlenfeldt <m...@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1206297}
---
M chrome/browser/ui/views/tabs/tab_drag_controller.cc
M chrome/browser/ui/views/tabs/tab_drag_controller_interactive_uitest.cc
M testing/buildbot/filters/ozone-linux.interactive_ui_tests_wayland.filter
M ui/base/test/ui_controls.h
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.cc
M ui/ozone/platform/wayland/emulate/wayland_input_emulate.h
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.cc
M ui/ozone/platform/wayland/test/wayland_ozone_ui_controls_test_helper.h
M ui/ozone/public/ozone_ui_controls_test_helper.cc
M ui/ozone/public/ozone_ui_controls_test_helper.h
M ui/views/test/ui_controls_factory_desktop_aura_ozone.cc
11 files changed, 259 insertions(+), 43 deletions(-)