[webkit-reviews] review granted: [Bug 185829] [css-masking] Fully support -webkit-clip-path on SVG elements : [Attachment 343897] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 185829: [css-masking] Fully support -webkit-clip-path on SVG elements
https://bugs.webkit.org/show_bug.cgi?id=185829

Attachment 343897: Patch

https://bugs.webkit.org/attachment.cgi?id=343897&action=review




--- Comment #12 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 343897
  --> https://bugs.webkit.org/attachment.cgi?id=343897
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343897&action=review

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:408
> +inline FloatRect clipPathReferenceBox(const RenderElement& renderer, const
CSSBoxType& boxType)

boxType can be passed by value. It's just an enum.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:410
> +    // Depends on https://bugs.webkit.org/show_bug.cgi?id=185797

This comment can be removed.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:411
> +    FloatRect referenceBox { };

No need for { }

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:413
> +    if (boxType == CSSBoxType::StrokeBox || boxType == CSSBoxType::BorderBox
> +	   || boxType == CSSBoxType::MarginBox)

Don't wrap the lines this narrow.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:417
> +    } else if (boxType == CSSBoxType::ViewBox && renderer.element()) {

Maybe this would be better as a switch, so the compiler tells you if you forgot
a value.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:421
> +	   referenceBox.setWidth(viewportSize.width());
> +	   referenceBox.setHeight(viewportSize.height());

Can this be setSize(viewportSize.width.size())?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:432
> +	   FloatRect referenceBox {clipPathReferenceBox(renderer,
clipPath.referenceBox())};

Would be simpler as FloatRect referenceBox = clipPathReferenceBox(renderer,
clipPath.referenceBox());

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:439
> +	   FloatRect referenceBox {clipPathReferenceBox(renderer,
clipPath.referenceBox())};

Same.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:442
> +	   return clipPath.pathForReferenceRect(FloatRoundedRect
{referenceBox}).contains(point);

Is the explicit FloatRoundedRect necessary?

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:453
> +	   FloatRect referenceBox {clipPathReferenceBox(renderer,
clipPath.referenceBox())};

Ditto.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:459
> +	   FloatRect referenceBox {clipPathReferenceBox(renderer,
clipPath.referenceBox())};
> +	   context.clipPath(clipPath.pathForReferenceRect(FloatRoundedRect
{referenceBox}));

Ditto.