Bug 35421

Summary: SVGResourceClipper needs to be moved to RenderSVGResourceClipper
Product: WebKit Reporter: Dirk Schulze <krit>
Component: SVGAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
move of SVGResourceClipper
abarth: review-
move of SVGResourceClipper zimmermann: review+

Description Dirk Schulze 2010-02-25 23:41:31 PST
SVGResourceClipper needs to follow Masker and must be moved to the new Renderer code. This gives more sense to the DRT results.
Comment 1 Dirk Schulze 2010-02-26 00:19:36 PST
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.
Comment 2 WebKit Review Bot 2010-02-26 00:20:29 PST
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.
Comment 3 WebKit Review Bot 2010-02-26 01:04:12 PST
Attachment 49569 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/314258
Comment 4 WebKit Review Bot 2010-02-26 01:11:08 PST
Attachment 49569 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/313361
Comment 5 Adam Barth 2010-02-26 01:38:23 PST
Comment on attachment 49569 [details]
move of SVGResourceClipper

Looks like you missed some of the project files.
Comment 6 Dirk Schulze 2010-02-26 06:58:41 PST
Created attachment 49580 [details]
move of SVGResourceClipper

issues fixed
Comment 7 Nikolas Zimmermann 2010-02-26 09:23:50 PST
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..
Comment 8 Dirk Schulze 2010-02-26 09:50:43 PST
(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.
Comment 9 Nikolas Zimmermann 2010-02-26 09:53:40 PST
(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 :-)
Comment 10 Dirk Schulze 2010-02-26 11:06:40 PST
Landed patch in r55289.