[webkit-reviews] review granted: [Bug 35421] SVGResourceClipper needs to be moved to RenderSVGResourceClipper : [Attachment 49580] move of SVGResourceClipper

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 35421: SVGResourceClipper needs to be moved to RenderSVGResourceClipper
https://bugs.webkit.org/show_bug.cgi?id=35421

Attachment 49580: move of SVGResourceClipper
https://bugs.webkit.org/attachment.cgi?id=49580&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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..