Support closest-corner/farthest-corner for circle()/ellipse() [chromium/src : main]
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
drive-by: this change should go through the I2S process with a runtime flag.
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
drive-by: this change should go through the I2S process with a runtime flag.
added a flag and filed a feature entry https://chromestatus.com/feature/5100672946143232 (will finish it till the end of the week)
Related details
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 2 comments![]( "Open in Gerrit")
args.Peek().Id())) {Merge this into the if () above for a single ConsumeIdent().
}Why not just:
```
float dx = closest ? std::min(dx0, dx1) : std::max(dx0, dx1);
float dy = closest ? std::min(dy0, dy1) : std::max(dy0, dy1);
return hypotf(dx, dy);
```
Related details
- Helmut Januschka
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. |
一丝 (Gerrit)
一丝 added 1 comment![]( "Open in Gerrit")
Helmut Januschkadrive-by: this change should go through the I2S process with a runtime flag.
added a flag and filed a feature entry https://chromestatus.com/feature/5100672946143232 (will finish it till the end of the week)
Please note that there is currently an issue with the specifications that needs to be resolved.
https://github.com/w3c/csswg-drafts/issues/10812
Related details
- Helmut Januschka
- Rune Lillesveen
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
- Fredrik Söderquist
- Helmut Januschka
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 1 comment![]( "Open in Gerrit")
This seems to introduce basically the same changes as that CL, could we resolve this first so that we don't have two outstanding changes for the same thing?
Related details
- Helmut Januschka
- Rune Lillesveen
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
Fredrik SöderquistLooping in fs@ who reviewed https://crrev.com/c/7754379
This seems to introduce basically the same changes as that CL, could we resolve this first so that we don't have two outstanding changes for the same thing?
Thanks. I had not seen [https://crrev.com/c/7754379](https://crrev.com/c/7754379) when I picked this up, and the bug was unassigned at the time, so I assigned it to myself and started from there.
I think [this-cl](https://crrev.com/c/7767079) may be the better consolidation point because it already includes the runtime flag / I2S, and it also updates the shape-outside code path in addition to the clip-path path.
That said, I do not want to have any bad mood or troubles. If you would prefer consolidating on [https://crrev.com/c/7754379](https://crrev.com/c/7754379) instead, I am going to step back, reassign the bug, and (if wanted) help fold the missing pieces over there. Please let me know which direction you would like me to take.
Merge this into the if () above for a single ConsumeIdent().
- Fredrik Söderquist
- Rune Lillesveen
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Why not just:
```
float dx = closest ? std::min(dx0, dx1) : std::max(dx0, dx1);
float dy = closest ? std::min(dy0, dy1) : std::max(dy0, dy1);
return hypotf(dx, dy);
```
Done
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 6 comments![]( "Open in Gerrit")
Fredrik SöderquistLooping in fs@ who reviewed https://crrev.com/c/7754379
Helmut JanuschkaThis seems to introduce basically the same changes as that CL, could we resolve this first so that we don't have two outstanding changes for the same thing?
Thanks. I had not seen [https://crrev.com/c/7754379](https://crrev.com/c/7754379) when I picked this up, and the bug was unassigned at the time, so I assigned it to myself and started from there.
I think [this-cl](https://crrev.com/c/7767079) may be the better consolidation point because it already includes the runtime flag / I2S, and it also updates the shape-outside code path in addition to the clip-path path.
That said, I do not want to have any bad mood or troubles. If you would prefer consolidating on [https://crrev.com/c/7754379](https://crrev.com/c/7754379) instead, I am going to step back, reassign the bug, and (if wanted) help fold the missing pieces over there. Please let me know which direction you would like me to take.
I agree with your take on completeness, and I did explicitly say that the other person [should assign the bug](https://chromium-review.googlesource.com/c/chromium/src/+/7754379/comments/9761b01b_5dbbd618). That CL was "stuck" waiting on CSS WG feedback though (as mentioned by Yisi) - something that would affect this CL as well. I will CC Hyowon.
float radius_x = radii.width();
float radius_y = radii.height();These are kind of unnecessary now - just use `radii` instead.
gfx::SizeF FloatSizeForRadiiInBox(const gfx::PointF& center,I think I'd just call this `ResolveRadii()` or something like that.
float FloatValueForRadiusInBox(const BasicShapeRadius&,
float center,
float box_width_or_height) const;This can be made private now.
if (radius.GetType() == BasicShapeRadius::kFarthestSide) {
return std::max(center, width_or_height_delta);
}
// closest-corner/farthest-corner require both axes for Euclidean distance.
// Use FloatSizeForRadiiInBox() for those values.
NOTREACHED();
}
gfx::SizeF BasicShapeEllipse::FloatSizeForRadiiInBox(
const gfx::PointF& center,
const gfx::SizeF& box_size) const {
auto resolve_radius = [&](const BasicShapeRadius& radius, float center_coord,
float box_dim) -> float {
if (radius.GetType() == BasicShapeRadius::kClosestCorner) {
return RadiusToCorner(center, box_size, /*closest=*/true);
}
if (radius.GetType() == BasicShapeRadius::kFarthestCorner) {
return RadiusToCorner(center, box_size, /*closest=*/false);
}
return FloatValueForRadiusInBox(radius, center_coord, box_dim);
};This looks awkward - why not just extend the existing function and then call it twice? (I.e quite similar to the circle case.)
const gfx::Vector2dF scaled_radius = gfx::ScaleVector2d(This can now just use `gfx::ScaleSize()` instead of passing through a `gfx::Vector2dF`.
Related details
- Helmut Januschka
- Rune Lillesveen
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. |
Hyowon Kim (Gerrit)
Hyowon Kim added 1 comment![]( "Open in Gerrit")
Fredrik SöderquistLooping in fs@ who reviewed https://crrev.com/c/7754379
Helmut JanuschkaThis seems to introduce basically the same changes as that CL, could we resolve this first so that we don't have two outstanding changes for the same thing?
Fredrik SöderquistThanks. I had not seen [https://crrev.com/c/7754379](https://crrev.com/c/7754379) when I picked this up, and the bug was unassigned at the time, so I assigned it to myself and started from there.
I think [this-cl](https://crrev.com/c/7767079) may be the better consolidation point because it already includes the runtime flag / I2S, and it also updates the shape-outside code path in addition to the clip-path path.
That said, I do not want to have any bad mood or troubles. If you would prefer consolidating on [https://crrev.com/c/7754379](https://crrev.com/c/7754379) instead, I am going to step back, reassign the bug, and (if wanted) help fold the missing pieces over there. Please let me know which direction you would like me to take.
I agree with your take on completeness, and I did explicitly say that the other person [should assign the bug](https://chromium-review.googlesource.com/c/chromium/src/+/7754379/comments/9761b01b_5dbbd618). That CL was "stuck" waiting on CSS WG feedback though (as mentioned by Yisi) - something that would affect this CL as well. I will CC Hyowon.
Thanks for looping me in. Happy to abandon mine. I left my thought on the spec/implementation mismatch as a comment on the issue.
https://github.com/w3c/csswg-drafts/issues/10812#issuecomment-4285488054
Thanks :)
Helmut Januschka (Gerrit)
Helmut Januschka added 5 comments![]( "Open in Gerrit")
float radius_x = radii.width();
float radius_y = radii.height();These are kind of unnecessary now - just use `radii` instead.
Done
gfx::SizeF FloatSizeForRadiiInBox(const gfx::PointF& center,I think I'd just call this `ResolveRadii()` or something like that.
Done
float FloatValueForRadiusInBox(const BasicShapeRadius&,
float center,
float box_width_or_height) const;This can be made private now.
Done
if (radius.GetType() == BasicShapeRadius::kFarthestSide) {
return std::max(center, width_or_height_delta);
}
// closest-corner/farthest-corner require both axes for Euclidean distance.
// Use FloatSizeForRadiiInBox() for those values.
NOTREACHED();
}
gfx::SizeF BasicShapeEllipse::FloatSizeForRadiiInBox(
const gfx::PointF& center,
const gfx::SizeF& box_size) const {
auto resolve_radius = [&](const BasicShapeRadius& radius, float center_coord,
float box_dim) -> float {
if (radius.GetType() == BasicShapeRadius::kClosestCorner) {
return RadiusToCorner(center, box_size, /*closest=*/true);
}
if (radius.GetType() == BasicShapeRadius::kFarthestCorner) {
return RadiusToCorner(center, box_size, /*closest=*/false);
}
return FloatValueForRadiusInBox(radius, center_coord, box_dim);
};This looks awkward - why not just extend the existing function and then call it twice? (I.e quite similar to the circle case.)
done. per-axis helper is reused for the simple cases, and
ResolveRadii() calls that logic for each axis while handling the corner cases centrally.
const gfx::Vector2dF scaled_radius = gfx::ScaleVector2d(This can now just use `gfx::ScaleSize()` instead of passing through a `gfx::Vector2dF`.
- 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")
Did you miss to upload a new patchset?
float radius_x = radii.width();
float radius_y = radii.height();Helmut JanuschkaThese are kind of unnecessary now - just use `radii` instead.
Done
- Helmut Januschka
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. |