[webkit-reviews] review denied: [Bug 96381] -webkit-clip-path should parse IRIs : [Attachment 165258] patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 96381: -webkit-clip-path should parse IRIs
https://bugs.webkit.org/show_bug.cgi?id=96381

Attachment 165258: patch
https://bugs.webkit.org/attachment.cgi?id=165258&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165258&action=review


Please use phrases on FIXME's. There is a build warning that you never run into
a code path in styleresolver.

> LayoutTests/css3/masking/clip-path-reference.html:21
> +<svg height="0">
> +    <clipPath id="c1" clipPathUnits="objectBoundingBox">

Please add a test with userSpaceOnUse as well. Note that you need to specify a
with and height for the SVG element then:
<svg height="200" width="200">
<clipPath id="c1" clipPathUnits="userSpaceOnUse">
    <rect x="20" y="20" width="160" height="160"/>
</clipPath>

> Source/WebCore/css/StyleBuilder.cpp:-1690
> -template <ClipPathOperation* (RenderStyle::*getterFunction)() const, void
(RenderStyle::*setterFunction)(PassRefPtr<ClipPathOperation>),
ClipPathOperation* (*initialFunction)()>
> -class ApplyPropertyClipPath {
> -public:

I was told to use this template, since it is the new way to use StyleBuilder.
Like discussed offline, the code from StyleResolver::createPathOperation should
be moved here.

> Source/WebCore/css/StyleBuilder.cpp:-2055
> -    setPropertyHandler(CSSPropertyWebkitClipPath,
ApplyPropertyClipPath<&RenderStyle::clipPath, &RenderStyle::setClipPath,
&RenderStyle::initialClipPath>::createHandler());

Needs to be added again.

> Source/WebCore/css/StyleResolver.cpp:5171
> +    // FIXME: needs handling for load pending SVG documents (similar to what
CSS Filters)

Add the reference to the bug report
https://bugs.webkit.org/show_bug.cgi?id=90405.

> Source/WebCore/rendering/RenderLayer.cpp:3149
> +	       // FIXME: doesn't work if we have an external reference to a SVG
document or if the SVG element is after the HTML one

The external referencing doesn't work in SVG either. We have a bug report about
that. The one with SVG after HTML should reference to the bug report:
https://bugs.webkit.org/show_bug.cgi?id=90405

> Source/WebCore/rendering/RenderLayer.cpp:3151
> +
> +

One empty line s enough.

> Source/WebCore/rendering/RenderLayer.cpp:3155
> +		   RenderSVGResourceClipper* clipper =
static_cast<RenderSVGResourceClipper*>(clipPath->renderer());
> +		   if (clipper)

If you don't have more checks, move the initialisation into the if function: 
if (RenderSVGResourceClipper* clipper =
static_cast<RenderSVGResourceClipper*>(clipPath->renderer()))

> Source/WebCore/rendering/RenderLayer.cpp:3156
> +		       clipper->applyClippingToContext(renderer(),
calculateLayerBounds(this, rootLayer, 0), paintDirtyRect,
transparencyLayerContext);

should be context, not transparencyLayerContext.

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.h:58
> +    // clipPath can be clipped too, but don't have a boundingBox or
repaintRect. So we can't call
> +    // applyResource directly and use the rects from the object, since they
are empty for RenderSVGResources
> +    // This function is public because we need to apply clipping on HTML
element
> +    bool applyClippingToContext(RenderObject*, const FloatRect&, const
FloatRect&, GraphicsContext*);

They should have a bounding box. Seems to be a bug.