SVGResourceClipper needs to follow Masker and must be moved to the new Renderer code. This gives more sense to the DRT results.
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.
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.
Attachment 49569 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/314258
Attachment 49569 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/313361
Comment on attachment 49569 [details] move of SVGResourceClipper Looks like you missed some of the project files.
Created attachment 49580 [details] move of SVGResourceClipper issues fixed
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..
(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.
(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 :-)
Landed patch in r55289.