Remove expired flag wayland-per-window-scaling [chromium/src : main]
Thomas Anderson (Gerrit)
Thomas Anderson voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Anderson (Gerrit)
Thomas Anderson voted Code-Review+1![]( "Open in Gerrit")
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 5 comments![]( "Open in Gerrit")
LGTM, but:
Typically I would prefer a CL which removed the flag, followed by a CL that updated the tests.
Since the flag was enabled on Linux, there should be no behavioral changes in Wayland tests when the flag is removed. Therefore I would not expect any test changes to be required (other than deleting references to the flag).
Perhaps I am misunderstanding.
const gfx::Rect expected_damage =Why is logic added here?
class WaylandScreenTestNoFractionalScale : public WaylandScreenTest {I would not change the name here; or if you must, since you are not changing anything, the correct usage is:
```
using WaylandScreenTestNoFractionalScale = WaylandScreenTest;
```
In general for these types of CLs we want to make as few changes as possible. You can always make changes like this in a follow-up.
INSTANTIATE_TEST_SUITE_P(XdgVersionStableTest,Why are these parameterized tests if there is only one value per test?
class WaylandWindowTestNoFractionalScale : public WaylandWindowTest {Again this could be a `using` statement.
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 5 comments![]( "Open in Gerrit")
LGTM, but:
Typically I would prefer a CL which removed the flag, followed by a CL that updated the tests.
Since the flag was enabled on Linux, there should be no behavioral changes in Wayland tests when the flag is removed. Therefore I would not expect any test changes to be required (other than deleting references to the flag).
Perhaps I am misunderstanding.
ty, I noticed that when removing the flag, tests are not running with the expectation that this flag is always enabled. So I updated the default params, in case future contributions run into unexpected behavior due to this param.
const gfx::Rect expected_damage =Why is logic added here?
`GetParam().supports_viewporter_surface_scaling` leads to scale factor being conveyed at a multiple of 120 per wayland spec, so the damage may become different due to rounding.
class WaylandScreenTestNoFractionalScale : public WaylandScreenTest {I would not change the name here; or if you must, since you are not changing anything, the correct usage is:
```
using WaylandScreenTestNoFractionalScale = WaylandScreenTest;
```In general for these types of CLs we want to make as few changes as possible. You can always make changes like this in a follow-up.
`WaylandScreenTestNoFractionalScale` should run with `.supports_viewporter_surface_scaling = false` config. Since I changed `.supports_viewporter_surface_scaling` to default true this test is now named differently. They don't really have meaning under `supports_viewporter_surface_scaling = true`.
INSTANTIATE_TEST_SUITE_P(XdgVersionStableTest,Why are these parameterized tests if there is only one value per test?
Historically they have 2 params (xdg stable vs xdg v6), and tests ran against all of them. But the params was removed with lacros.
class WaylandWindowTestNoFractionalScale : public WaylandWindowTest {Again this could be a `using` statement.
- Dana Fried
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nick Yamane (Gerrit)
Nick Yamane added 2 comments![]( "Open in Gerrit")
// this needs to set scale with the fractional scale manager as wellthis seems incomplete or mistankenly placed?
Related details
- Dana Fried
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 5 comments![]( "Open in Gerrit")
Kramer GeLGTM, but:
Typically I would prefer a CL which removed the flag, followed by a CL that updated the tests.
Since the flag was enabled on Linux, there should be no behavioral changes in Wayland tests when the flag is removed. Therefore I would not expect any test changes to be required (other than deleting references to the flag).
Perhaps I am misunderstanding.
ty, I noticed that when removing the flag, tests are not running with the expectation that this flag is always enabled. So I updated the default params, in case future contributions run into unexpected behavior due to this param.
If the flag was on by default, then it's possible that fieldtrial_testing_config.json was incorrectly configured to turn it off.
That's very weird and probably bad? What tests were failing and on what platform?
In general, I'm uncomfortable that removing a flag that was on by default should cause any tests to fail.
Is it possible you are inadvertently enabling the flag on a platform that it was not previously enabled on, either in code or by the testing config?
const gfx::Rect expected_damage =Kramer GeWhy is logic added here?
`GetParam().supports_viewporter_surface_scaling` leads to scale factor being conveyed at a multiple of 120 per wayland spec, so the damage may become different due to rounding.
Acknowledged
class WaylandScreenTestNoFractionalScale : public WaylandScreenTest {Kramer GeI would not change the name here; or if you must, since you are not changing anything, the correct usage is:
```
using WaylandScreenTestNoFractionalScale = WaylandScreenTest;
```In general for these types of CLs we want to make as few changes as possible. You can always make changes like this in a follow-up.
`WaylandScreenTestNoFractionalScale` should run with `.supports_viewporter_surface_scaling = false` config. Since I changed `.supports_viewporter_surface_scaling` to default true this test is now named differently. They don't really have meaning under `supports_viewporter_surface_scaling = true`.
Acknowledged
INSTANTIATE_TEST_SUITE_P(XdgVersionStableTest,Kramer GeWhy are these parameterized tests if there is only one value per test?
Historically they have 2 params (xdg stable vs xdg v6), and tests ran against all of them. But the params was removed with lacros.
Acknowledged
class WaylandWindowTestNoFractionalScale : public WaylandWindowTest {Kramer GeAgain this could be a `using` statement.
This is same as the `*NoFractionalScale` test.
- Kramer Ge
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge added 3 comments![]( "Open in Gerrit")
Kramer GeLGTM, but:
Typically I would prefer a CL which removed the flag, followed by a CL that updated the tests.
Since the flag was enabled on Linux, there should be no behavioral changes in Wayland tests when the flag is removed. Therefore I would not expect any test changes to be required (other than deleting references to the flag).
Perhaps I am misunderstanding.
Dana Friedty, I noticed that when removing the flag, tests are not running with the expectation that this flag is always enabled. So I updated the default params, in case future contributions run into unexpected behavior due to this param.
If the flag was on by default, then it's possible that fieldtrial_testing_config.json was incorrectly configured to turn it off.
That's very weird and probably bad? What tests were failing and on what platform?
In general, I'm uncomfortable that removing a flag that was on by default should cause any tests to fail.
Is it possible you are inadvertently enabling the flag on a platform that it was not previously enabled on, either in code or by the testing config?
`fieldtrial_testing_config.json` does turn it on, but the issue is with the test framework, `supports_viewporter_surface_scaling` is a test param `=false` which [disables](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/test_wayland_server_thread.cc;l=116;drc=796e6996a524533ef2af2ab09c8dd541368f491b;bpv=0;bpt=1) the wayland extension. So we can't use the feature even though it is on, unfortunately.
The tests are only run on linux.
Removing the flag does not cause tests to fail, but they do not test the default on behavior, so in this patch I attempt to make tests reflect reality.
// this needs to set scale with the fractional scale manager as wellthis seems incomplete or mistankenly placed?
- Dana Fried
- Nick Yamane
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dana Fried
| Code-Review | +1 |
Kramer GeLGTM, but:
Typically I would prefer a CL which removed the flag, followed by a CL that updated the tests.
Since the flag was enabled on Linux, there should be no behavioral changes in Wayland tests when the flag is removed. Therefore I would not expect any test changes to be required (other than deleting references to the flag).
Perhaps I am misunderstanding.
Dana Friedty, I noticed that when removing the flag, tests are not running with the expectation that this flag is always enabled. So I updated the default params, in case future contributions run into unexpected behavior due to this param.
Kramer GeIf the flag was on by default, then it's possible that fieldtrial_testing_config.json was incorrectly configured to turn it off.
That's very weird and probably bad? What tests were failing and on what platform?
In general, I'm uncomfortable that removing a flag that was on by default should cause any tests to fail.
Is it possible you are inadvertently enabling the flag on a platform that it was not previously enabled on, either in code or by the testing config?
`fieldtrial_testing_config.json` does turn it on, but the issue is with the test framework, `supports_viewporter_surface_scaling` is a test param `=false` which [disables](https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/test/test_wayland_server_thread.cc;l=116;drc=796e6996a524533ef2af2ab09c8dd541368f491b;bpv=0;bpt=1) the wayland extension. So we can't use the feature even though it is on, unfortunately.
The tests are only run on linux.
Removing the flag does not cause tests to fail, but they do not test the default on behavior, so in this patch I attempt to make tests reflect reality.
- Kramer Ge
- Nick Yamane
- Thomas Anderson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kramer Ge (Gerrit)
Kramer Ge voted and added 1 comment![]( "Open in Gerrit")
- Nick Yamane
- Thomas Anderson
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Remove expired flag wayland-per-window-scaling
This flag is on expired in m138, been on for linux by default.
Wayland unittests are now running assuming fractional_scale will
directly send scale to each surface as default, some tests are left in
old config as they test deducing scale by looking at the which display
they're on.
- M chrome/browser/about_flags.cc
- M chrome/browser/flag-metadata.json
- M chrome/browser/flag_descriptions.h
- M ui/base/ui_base_features.cc
- M ui/base/ui_base_features.h
- M ui/ozone/platform/wayland/gpu/wayland_surface_factory_unittest.cc
- M ui/ozone/platform/wayland/host/wayland_connection.cc
- M ui/ozone/platform/wayland/host/wayland_data_drag_controller_unittest.cc
- M ui/ozone/platform/wayland/host/wayland_output_unittest.cc
- M ui/ozone/platform/wayland/host/wayland_screen_unittest.cc
- M ui/ozone/platform/wayland/host/wayland_surface.h
- M ui/ozone/platform/wayland/host/wayland_window_drag_controller_unittest.cc
- M ui/ozone/platform/wayland/host/wayland_window_unittest.cc
- M ui/ozone/platform/wayland/test/mock_surface.cc
- M ui/ozone/platform/wayland/test/test_fractional_scale.cc
- M ui/ozone/platform/wayland/test/test_wayland_server_thread.h
Code-Review: +1 by Dana Fried
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |