Summary: | SVGResourceClipper needs to be moved to RenderSVGResourceClipper | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||
Component: | SVG | Assignee: | 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
Dirk Schulze
2010-02-25 23:41:31 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.
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. |