[Webkit-unassigned] [Bug 35421] SVGResourceClipper needs to be moved to RenderSVGResourceClipper

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=35421


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #49580|review?                     |review+
               Flag|                            |




--- Comment #7 from Nikolas Zimmermann <zimmermann at kde.org>  2010-02-26 09:23:50 PST ---
(From update of attachment 49580)
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..

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.