[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