Bug 233344

<rdar://problem/85611980>

Created attachment 456773 [details] wip

Created attachment 458841 [details] Patch

Comment on attachment 458841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458841&action=review > Source/WebCore/rendering/PathOperation.cpp:58 > +static double toPositiveAngle(double angle) > +{ > + angle = fmod(angle, 360); > + while (angle < 0) > + angle += 360.0; > + return angle; > +} This is the same as this code in CSSGradientAngle: angleDeg = fmodf(angleDeg, 360); if (angleDeg < 0) angleDeg += 360; > Source/WebCore/rendering/PathOperation.cpp:60 > +double RayPathOperation::getLengthForPath() const We don't use 'get' in function names, so just 'lengthForPath()" > Source/WebCore/rendering/PathOperation.cpp:86 > + // Get possible intersection sides > + double s1 = cos(radians) >= 0 ? top : bottom; > + double s2 = sin(radians) >= 0 ? right : left; > + // Get intersection side - based on length of tan() * s1 > + double intersectionSide = sin(radians) * s1 > cos(radians) * s2 ? s1 : s2; If you can factor bits of this into reusable functions in GeometryUtilities.cpp it would be easier to read. > Source/WebCore/rendering/PathOperation.cpp:88 > + double angle = deg2rad(sin(radians) * s1 > cos(radians) * s2 ? 90 - std::fmod(toPositiveAngle(m_angle), 90) : std::fmod(toPositiveAngle(m_angle), 90)); Would prefer not to wrap that very complex expression in deg2rad(), for readability. Use an intermediate variable. > Source/WebCore/rendering/PathOperation.h:230 > float m_angle; This should be initialized: float m_angle { 0 } > Source/WebCore/rendering/PathOperation.h:232 > bool m_isContaining; bool m_isContaining { false } > Source/WebCore/rendering/RenderLayer.cpp:1326 > + } else if (pathOperation && is<RayPathOperation>(pathOperation)) { I'm surprised that pathOperation is in scope here. This function would be better with earlier returns: auto pathOperation = renderer().style().offsetPath() if (!pathOperation) return; if (is<BoxPathOperation>(*pathOperation)) { ... return; } if (is<RayPathOperation>(*operation) { ... return; } > Source/WebCore/rendering/RenderLayer.h:511 > + whitespace

Created attachment 458919 [details] Patch

(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 458841 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458841&action=review > > > Source/WebCore/rendering/PathOperation.cpp:58 > > +static double toPositiveAngle(double angle) > > +{ > > + angle = fmod(angle, 360); > > + while (angle < 0) > > + angle += 360.0; > > + return angle; > > +} > > This is the same as this code in CSSGradientAngle: > angleDeg = fmodf(angleDeg, 360); > if (angleDeg < 0) > angleDeg += 360; Added shared function in GeometryUtilities. > > > Source/WebCore/rendering/PathOperation.cpp:60 > > +double RayPathOperation::getLengthForPath() const > > We don't use 'get' in function names, so just 'lengthForPath()" > Fixed. > > Source/WebCore/rendering/PathOperation.cpp:86 > > + // Get possible intersection sides > > + double s1 = cos(radians) >= 0 ? top : bottom; > > + double s2 = sin(radians) >= 0 ? right : left; > > + // Get intersection side - based on length of tan() * s1 > > + double intersectionSide = sin(radians) * s1 > cos(radians) * s2 ? s1 : s2; > > If you can factor bits of this into reusable functions in > GeometryUtilities.cpp it would be easier to read. > Added 3 functions in GeometryUtilities that calculate the intermediate values and the final value. Added another function that is used earlier here. > > Source/WebCore/rendering/PathOperation.cpp:88 > > + double angle = deg2rad(sin(radians) * s1 > cos(radians) * s2 ? 90 - std::fmod(toPositiveAngle(m_angle), 90) : std::fmod(toPositiveAngle(m_angle), 90)); > > Would prefer not to wrap that very complex expression in deg2rad(), for > readability. Use an intermediate variable. > Fixed. > > Source/WebCore/rendering/PathOperation.h:230 > > float m_angle; > > This should be initialized: float m_angle { 0 } > Fixed. > > Source/WebCore/rendering/PathOperation.h:232 > > bool m_isContaining; > > bool m_isContaining { false } > Fixed. > > Source/WebCore/rendering/RenderLayer.cpp:1326 > > + } else if (pathOperation && is<RayPathOperation>(pathOperation)) { > > I'm surprised that pathOperation is in scope here. > > This function would be better with earlier returns: > > auto pathOperation = renderer().style().offsetPath() > if (!pathOperation) > return; > > if (is<BoxPathOperation>(*pathOperation)) { > ... > return; > } > > if (is<RayPathOperation>(*operation) { > ... > return; > } Fixed > > > Source/WebCore/rendering/RenderLayer.h:511 > > + > > whitespace Fixed.

Comment on attachment 458919 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458919&action=review > Source/WebCore/platform/graphics/GeometryUtilities.h:69 > +double lengthOfRayIntersectionWithBoundingBox(const FloatRect&, const FloatSize&, double); Name the arguments. > Source/WebCore/platform/graphics/GeometryUtilities.h:73 > +double lengthOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double); I can't tell what the arguments are from the signature. I'd expect a FloatRect, and a FloatPoint + FloatSize for the ray? > Source/WebCore/platform/graphics/GeometryUtilities.h:77 > +double angleOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double, double); Name the arguments (clarifying if angles are degrees or radians) > Source/WebCore/platform/graphics/GeometryUtilities.h:80 > +std::tuple<double, double, double, double> distanceOfPointToSidesOfRect(const FloatRect&, const FloatSize&); Consider RectEdges<double>

Created attachment 458923 [details] Patch

(In reply to Simon Fraser (smfr) from comment #7) > Comment on attachment 458919 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458919&action=review > > > Source/WebCore/platform/graphics/GeometryUtilities.h:69 > > +double lengthOfRayIntersectionWithBoundingBox(const FloatRect&, const FloatSize&, double); > > Name the arguments. > Fixed. > > Source/WebCore/platform/graphics/GeometryUtilities.h:73 > > +double lengthOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double); > > I can't tell what the arguments are from the signature. I'd expect a > FloatRect, and a FloatPoint + FloatSize for the ray? > Fixed. > > Source/WebCore/platform/graphics/GeometryUtilities.h:77 > > +double angleOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double, double); > > Name the arguments (clarifying if angles are degrees or radians) > Fixed. > > Source/WebCore/platform/graphics/GeometryUtilities.h:80 > > +std::tuple<double, double, double, double> distanceOfPointToSidesOfRect(const FloatRect&, const FloatSize&); > > Consider RectEdges<double> Fixed.

Comment on attachment 458923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458923&action=review > Source/WebCore/platform/graphics/GeometryUtilities.h:69 > +double lengthOfRayIntersectionWithBoundingBox(const FloatRect& boundingRect, const FloatSize& position, double angle); Normally a position would be a FloatPoint, but I guess the position is relative to the rect location? I think it would be better as a FloatPoint in the same coordinate space as the FloatRect. You have 'angle' here and 'radians' below. I'd suggest 'angleInRadians'. You could also do a `using std::pair<FloatPosition, double> = Ray` which might make these clearer. > Source/WebCore/platform/graphics/GeometryUtilities.h:73 > +double lengthOfPointToSideOfIntersection(const FloatRect& boundingRect, const FloatSize& position, double radians); Same comments as above. > Source/WebCore/platform/graphics/GeometryUtilities.h:76 > +// Given a box and a ray (described by a starting position and angle in degrees), compute the acute angle between the ray > +// and the line segment from the starting point to the closest point on the side that the ray intersects with. Is the 'length' argument to this function the value returned from lengthOfPointToSideOfIntersection()? Can we just compute it inside this function?

Created attachment 458930 [details] Patch

(In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 458923 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458923&action=review > > > Source/WebCore/platform/graphics/GeometryUtilities.h:69 > > +double lengthOfRayIntersectionWithBoundingBox(const FloatRect& boundingRect, const FloatSize& position, double angle); > > Normally a position would be a FloatPoint, but I guess the position is > relative to the rect location? I think it would be better as a FloatPoint in > the same coordinate space as the FloatRect. > > You have 'angle' here and 'radians' below. I'd suggest 'angleInRadians'. You > could also do a `using std::pair<FloatPosition, double> = Ray` which might > make these clearer. > I think std::pair works well here. > > Source/WebCore/platform/graphics/GeometryUtilities.h:73 > > +double lengthOfPointToSideOfIntersection(const FloatRect& boundingRect, const FloatSize& position, double radians); > > Same comments as above. > Fixed. > > Source/WebCore/platform/graphics/GeometryUtilities.h:76 > > +// Given a box and a ray (described by a starting position and angle in degrees), compute the acute angle between the ray > > +// and the line segment from the starting point to the closest point on the side that the ray intersects with. > > Is the 'length' argument to this function the value returned from > lengthOfPointToSideOfIntersection()? Can we just compute it inside this > function? Now computed inside instead of passed.

Created attachment 458987 [details] Patch

Created attachment 459087 [details] Patch

Created attachment 459092 [details] Patch

Committed r294001 (250437@main): <https://commits.webkit.org/250437@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459092 [details].