Add flag and initial work for SVG New Zoom [chromium/src : main]
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
Looks like we'll return at the same date... 😊
No need to approach this before then. For the most part this is either adding or removing zoom, but there may be the occasional slightly bigger change where that approach didn't quite work. This is of course not considered to be close to finished, but is mainly uploaded to enable sharing.
Related details
- Philip Rogers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Prashant Nevase (Gerrit)
Prashant Nevase added 1 comment![]( "Open in Gerrit")
Quite a big cl though. Is it possible to have small writeup?
I'm not sure, but it is possible to do changes only in painters and achieve the same or to get zoom information in painters, layout objects are required to be modified?
Related details
- Fredrik Söderquist
- Philip Rogers
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. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
name: "SvgNewZoom",What are your thoughts around testing?
We could add a internals (non-wpt) web test like:
```
<!doctype html>
<html style="zoom: 2;>
// force-enable feature flag via JS
<svg>
<!-- basic case -->
<rect x="10" y="10" width="10" height="10" fill="blue"></rect>
<!-- clip path -->
<rect x="10" y="30" width="10" height="10" fill="blue" clip-path...></rect>
<!-- ... etc, for every line of this patch ? -->
</svg>
</html>
```
The -expected.html could then be the same, except with the feature force-disabled.
For web tests, we could add two virtual test suites: scalefactor200+SVGNewZoom and scalefactor200-SVGNewZoom.
It may be possible to add this to third_party/blink/renderer/platform/testing/paint_test_configurations.h, for the few tests that cover zoom.
I'd like to avoid toil around baselines. We could do a one-off local-only test of svg pixel tests with the feature disabled and zoom forced to 2, commit those baselines, and then enable the feature and ensure the tests still pass.
Related details
- Fredrik Söderquist
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
Quite a big cl though. Is it possible to have small writeup?
I'm not sure, but it is possible to do changes only in painters and achieve the same or to get zoom information in painters, layout objects are required to be modified?
Only doing this post-layout is essentially what we're doing now - just with the "barrier" moved a bit. I think that would have largely the same issues as the current implementation.
The bug has a small write-up.
name: "SvgNewZoom",What are your thoughts around testing?
We could add a internals (non-wpt) web test like:
```
<!doctype html>
<html style="zoom: 2;>
// force-enable feature flag via JS
<svg>
<!-- basic case -->
<rect x="10" y="10" width="10" height="10" fill="blue"></rect>
<!-- clip path -->
<rect x="10" y="30" width="10" height="10" fill="blue" clip-path...></rect>
<!-- ... etc, for every line of this patch ? -->
</svg>
</html>
```The -expected.html could then be the same, except with the feature force-disabled.
For web tests, we could add two virtual test suites: scalefactor200+SVGNewZoom and scalefactor200-SVGNewZoom.
It may be possible to add this to third_party/blink/renderer/platform/testing/paint_test_configurations.h, for the few tests that cover zoom.
I'd like to avoid toil around baselines. We could do a one-off local-only test of svg pixel tests with the feature disabled and zoom forced to 2, commit those baselines, and then enable the feature and ensure the tests still pass.
What are your thoughts around testing?
I had initially hoped that the hidpi would be useful for this, but I don't feel too confident about that now.
> We could add a internals (non-wpt) web test like:
> ```
> <!doctype html>
> <html style="zoom: 2;>
> // force-enable feature flag via JS
> <svg>
> <!-- basic case -->
> <rect x="10" y="10" width="10" height="10" fill="blue"></rect>
>
> <!-- clip path -->
> <rect x="10" y="30" width="10" height="10" fill="blue" clip-path...></rect>
>
> <!-- ... etc, for every line of this patch ? -->
> </svg>
> </html>
> ```
>
> The -expected.html could then be the same, except with the feature force-disabled.
We could do that, but it will of course only cover "known" cases, and for some "lines" the set of possible paths to it might be large (and difficult to enumerate).
For web tests, we could add two virtual test suites: scalefactor200+SVGNewZoom and scalefactor200-SVGNewZoom.
This `scalefactor200*` would be independent from the existing one then I take it? I.e the set of tests would presumably be different. (The existing one doesn't reach that widely I think?)
It may be possible to add this to third_party/blink/renderer/platform/testing/paint_test_configurations.h, for the few tests that cover zoom.
I guess that will be trade-off between runtime of an additional configuration vs. how much additional coverage. I see that we recently dropped one config, so I guess we may have runtime budget again =P.
I'd like to avoid toil around baselines. We could do a one-off local-only test of svg pixel tests with the feature disabled and zoom forced to 2, commit those baselines, and then enable the feature and ensure the tests still pass.
- Philip Rogers
- Prashant Nevase
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. |
Philip Rogers (Gerrit)
Philip Rogers added 2 comments![]( "Open in Gerrit")
Strong support for this patch by the way. It's fixing an early design mistake and pays down a big source of tech debt. Could you add some basic test coverage, and then I'll do a line-by-line review?
We could add a internals (non-wpt) web test like:
We could do that, but it will of course only cover "known" cases, and for some "lines" the set of possible paths to it might be large (and difficult to enumerate).
Good point. Maybe it will be better to just use the virtual test suites for this. So, let's not do my suggestion of a new one-off test like this.
> For web tests, we could add two virtual test suites: scalefactor200+SVGNewZoom and scalefactor200-SVGNewZoom.This `scalefactor200*` would be independent from the existing one then I take it? I.e the set of tests would presumably be different. (The existing one doesn't reach that widely I think?)
I think it would be good to have the non-default codepaths tested in case we need to emergency-disable this in the future, but we could use the existing scalefactor200 suite, and then add a second suite for the non-default configuration?
In other words:
1. Pick out some selection of svg tests that would provide good test coverage. This could be a small number of hand-selected tests, or large directories which have svg tests, or something in-between.
2. Add those tests to the existing scalefactor200 suite
3. Add a new virtual test suite, scalefactor200NonDefaultSVGNewZoom, which runs those tests with the non-default SVGNewZoom value.
WDYT?
> It may be possible to add this to third_party/blink/renderer/platform/testing/paint_test_configurations.h, for the few tests that cover zoom.I guess that will be trade-off between runtime of an additional configuration vs. how much additional coverage. I see that we recently dropped one config, so I guess we may have runtime budget again =P.
I'd like to avoid toil around baselines. We could do a one-off local-only test of svg pixel tests with the feature disabled and zoom forced to 2, commit those baselines, and then enable the feature and ensure the tests still pass.
Yes, this sounds workable.
- Fredrik Söderquist
- Prashant Nevase
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. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
name: "SvgNewZoom",Will this feature allow compositing SVG's with non-1 `EffectiveZoom`? If so, can this change also add a comment in this CL to `third_party/blink/renderer/core/animation/compositor_animations.cc` to remove the `else if (layout_object->StyleRef().EffectiveZoom() != 1)` once this feature is enabled?
The reason this was added was due to a "double-zoom", which I believe this approach will fix, see https://issues.chromium.org/issues/40172437.
Adding a comment with this CL will prevent it from slipping through the cracks again. Note that `EffectiveZoom` gets set to a non-1 value any time a display's DPI is scaled, so *many* users haven't been getting compositor-accelerated SVG animations for years 😞
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 1 comment![]( "Open in Gerrit")
name: "SvgNewZoom",Will this feature allow compositing SVG's with non-1 `EffectiveZoom`? If so, can this change also add a comment in this CL to `third_party/blink/renderer/core/animation/compositor_animations.cc` to remove the `else if (layout_object->StyleRef().EffectiveZoom() != 1)` once this feature is enabled?
The reason this was added was due to a "double-zoom", which I believe this approach will fix, see https://issues.chromium.org/issues/40172437.
Adding a comment with this CL will prevent it from slipping through the cracks again. Note that `EffectiveZoom` gets set to a non-1 value any time a display's DPI is scaled, so *many* users haven't been getting compositor-accelerated SVG animations for years 😞
I had a separate change that worked around this issue (see https://chromium-review.googlesource.com/c/chromium/src/+/7547998), but your approach is better, so I abandoned it.
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 1 comment![]( "Open in Gerrit")
name: "SvgNewZoom",Kurt Catti-SchmidtWill this feature allow compositing SVG's with non-1 `EffectiveZoom`? If so, can this change also add a comment in this CL to `third_party/blink/renderer/core/animation/compositor_animations.cc` to remove the `else if (layout_object->StyleRef().EffectiveZoom() != 1)` once this feature is enabled?
The reason this was added was due to a "double-zoom", which I believe this approach will fix, see https://issues.chromium.org/issues/40172437.
Adding a comment with this CL will prevent it from slipping through the cracks again. Note that `EffectiveZoom` gets set to a non-1 value any time a display's DPI is scaled, so *many* users haven't been getting compositor-accelerated SVG animations for years 😞
I had a separate change that worked around this issue (see https://chromium-review.googlesource.com/c/chromium/src/+/7547998), but your approach is better, so I abandoned it.
Yep, (painfully) aware of that. I will add a comment there when respin this.
Related details
- Kurt Catti-Schmidt
- Prashant Nevase