Bug 35421 – SVGResourceClipper needs to be moved to RenderSVGResourceClipper

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVGResourceClipper needs to follow Masker and must be moved to the new Renderer code. This gives more sense to the DRT results.

[Dirk Schulze]

Created attachment 49569 [details]
move of SVGResourceClipper

This moves the current clipper code to RenderSVGResourceClipper. No fixes of the current code so far but I'm also working on a complete implementation. For now it's better not to change the code, to remove the SVGResources faster.

[WebKit Review Bot]

Attachment 49569 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/svg/graphics/SVGResourceClipper.h': Can't open for reading
Skipping input 'WebCore/svg/graphics/SVGResourceClipper.cpp': Can't open for reading
WebCore/rendering/RenderSVGResourceClipper.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/rendering/RenderSVGResourceClipper.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 49569 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/314258

[WebKit Review Bot]

Attachment 49569 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/313361

[Adam Barth]

Comment on attachment 49569 [details]
move of SVGResourceClipper

Looks like you missed some of the project files.

[Dirk Schulze]

Created attachment 49580 [details]
move of SVGResourceClipper

issues fixed

[Nikolas Zimmermann]

Comment on attachment 49580 [details]
move of SVGResourceClipper

Looks great, r=me. Some small comments before landing:



> +    // apply Clipper
> +    if (RenderSVGResourceClipper* clipper = getRenderSVGResourceById<RenderSVGResourceClipper>(document, clipperId))
> +        clipper->applyResource(object, paintInfo.context);
> +    else if (!clipperId.isEmpty())
>          svgElement->document()->accessSVGExtensions()->addPendingResource(clipperId, styledElement);

The comment is not needed, please remove.

> +    // We only have the renderer for masker and clipper at the moment.
> +    if (RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(object->document(), object->style()->svgStyle()->maskElement()))
> +        masker->invalidateClient(object);
> +    if (RenderSVGResourceClipper* clipper = getRenderSVGResourceById<RenderSVGResourceClipper>(object->document(), object->style()->svgStyle()->clipPath()))
> +        clipper->invalidateClient(object);

Ok, you clearly want to return in the first if - if you know it's a masker, it can't be a clipper. So please add returns in the if clauses.

> @@ -497,6 +511,11 @@ void writeSVGResource(TextStream& ts, co
>          writeNameValuePair(ts, "maskUnits", masker->maskUnits());
>          writeNameValuePair(ts, "maskContentUnits", masker->maskContentUnits());
>      }
> +    if (resource->resourceType() == ClipperResourceType) {
> +        RenderSVGResourceClipper* clipper = static_cast<RenderSVGResourceClipper*>(resource);
> +        ASSERT(clipper);
> +        writeNameValuePair(ts, "clipPathUnits", clipper->clipPathUnits());
> +    }

Hm, you can use "else if" here, no? Same reason as above.


>      if (RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(document, svgStyle->maskElement()))
>          masker->invalidateClient(object);
>  
> -    SVGResourceClipper* clipper = getClipperById(document, svgStyle->clipPath(), object);
> -    if (clipper)
> -        clipper->invalidate();
> +    if (RenderSVGResourceClipper* clipper = getRenderSVGResourceById<RenderSVGResourceClipper>(document, svgStyle->clipPath()))
> +        clipper->invalidateClient(object);

Same as above. Early returns to save unnecesary casts..

[Dirk Schulze]

(In reply to comment #7)
You may have missunderstood the relevant parts in SVGRenderSupport and SVGStyledElement. An element changes state, so we have to invalidate _all_ resources of it, not just the first hit.
But you're right with TreeAsText. Will fix this one.

[Nikolas Zimmermann]

(In reply to comment #8)
> (In reply to comment #7)
> You may have missunderstood the relevant parts in SVGRenderSupport and
> SVGStyledElement. An element changes state, so we have to invalidate _all_
> resources of it, not just the first hit.
> But you're right with TreeAsText. Will fix this one.

Oh my, sure you're right :-)

[Dirk Schulze]

Landed patch in r55289.