[webkit-reviews] review granted: [Bug 233344] Support ray() shape in offset-path : [Attachment 458841] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 5 13:20:41 PDT 2022
Simon Fraser (smfr) <simon.fraser at apple.com> has granted Nikos Mouchtaris
<nmouchtaris at apple.com>'s request for review:
Bug 233344: Support ray() shape in offset-path
https://bugs.webkit.org/show_bug.cgi?id=233344
Attachment 458841: Patch
https://bugs.webkit.org/attachment.cgi?id=458841&action=review
--- Comment #4 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 458841
--> https://bugs.webkit.org/attachment.cgi?id=458841
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
More information about the webkit-reviews
mailing list