Bug 35020 – Move SVGResources to Renderers, starting with Masker

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

Created attachment 48866 [details]
Patch

We have rendering specific code in WebCore/svg/graphics. The goal is to move this code into suitable Renderers. This helps us to clean up the code and makes maintenance easier. It also makes it possible to remove rendering specific code from SVG*Elements into this renderers. So the Renderer contains evet is needed to use the resource. This patch starts moving SVGResourceMasker to RenderSVGResourceMasker.

[Dirk Schulze]

Comment on attachment 48866 [details]
Patch

Need to practice with webkit-patch a bit more :-P

[WebKit Review Bot]

Attachment 48866 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.h': Can't open for reading
WebCore/rendering/RenderSVGResource.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.cpp': Can't open for reading
WebCore/rendering/RenderSVGResourceMasker.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

Created attachment 48867 [details]
Patch

[WebKit Review Bot]

Attachment 48867 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.h': Can't open for reading
WebCore/rendering/RenderSVGResource.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.cpp': Can't open for reading
WebCore/rendering/RenderSVGResourceMasker.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 48867 [details]
Patch

Moin Dirk,

okay, the ChangeLog really needs a rewrite :-) I'd like to see more comments, what happened per file.
Explain the RenderSVGResource concept, that it will serve as base class for all upcoming RenderSVGResourceXXX objects, that it will replace SVGResource - that we need to keep SVGResource until the transition is finished, leading to a bit confusing code, until the move is done :-)

> Index: WebCore/Android.mk
> +    rendering/RenderSVGResourceMasker.cpp \

Tabs.

> Index: WebCore/rendering/RenderSVGResource.h
> +enum RenderSVGResourceType {
> +    // Painting mode
> +    MaskerResourceType = 0
> +};

The comment is confusing, please remove. Also the 0 default is not needed.

> +
> +class RenderSVGResource : public RenderSVGHiddenContainer {
> +
> +public:

One newline too much

> +    virtual const char* renderName() const { return "RenderSVGResource"; }

I'd either not implement renderName() or make it pure virtual. As RenderSVGResource is not constructable, this doesn't make much sense.

> Index: WebCore/rendering/SVGRenderSupport.cpp
> -    SVGResourceClipper* clipper = getClipperById(document, clipperId, object);
> -    SVGResourceMasker* masker = getMaskerById(document, maskerId, object);
> -
> +    // apply Masker
> +    RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(document, maskerId);
>      if (masker) {
Please fold the statment above into the if().

> +    SVGResourceClipper* clipper = getClipperById(document, clipperId, object);
>      if (clipper) {

Ditto.

>  FloatRect SVGRenderBase::maskerBoundingBoxForRenderer(const RenderObject* object) const
>  {
> -    SVGResourceMasker* masker = getMaskerById(object->document(), object->style()->svgStyle()->maskElement(), object);
> +    RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(object->document(), object->style()->svgStyle()->maskElement());
>      if (masker)
> -        return masker->maskerBoundingBox(object->objectBoundingBox());

Ditto.

> Index: WebCore/rendering/SVGRenderTreeAsText.cpp
> +void writeSVGResource(TextStream& ts, const RenderObject& object, int indent)

> +
> +    RenderSVGResource* resource = const_cast<RenderObject&>(object).toRenderSVGResource();
> +    if (resource->resourceType() == MaskerResourceType) {
> +        RenderSVGResourceMasker* masker = static_cast<RenderSVGResourceMasker*>(resource);
> +        if (!masker)
> +            return;

You want to ASSERT(masker) instead of early-return.

> +    // FIXME: Add the other SVGResources here.
Please change to comment to something like "Handle other RenderSVGResource* classes, as soon as they are converted from SVGResource*"

> +void writeRenderSVGResource(TextStream& ts, const RenderObject& object, int indent)
How about naming the function writeResources instaed of writeRenderSVGResource?

> +
> +    if (!svgStyle->maskElement().isEmpty()) {
> +        writeIndent(ts, indent);
> +        ts << " ";
> +        writeNameAndQuotedValue(ts, "masker", svgStyle->maskElement());
> +        RenderSVGResourceMasker* masker = getRenderSVGResourceById<RenderSVGResourceMasker>(object.document(), svgStyle->maskElement());
> +        if (masker) {
> +            ts << " ";
> +            writeStandardPrefix(ts, *masker, 0);
> +            ts << " " << masker->resourceBoundingBox(object.objectBoundingBox()) << "\n";
> +        }
> +    }

Hmm this seems flawed, we shouldn't dump "masker=".. anything, if the maskElement() does NOT point to an actual mask element, but say a clipPath.
Fold the RenderSVGResourceMasker* masker = get... line with the if() and only writeIndent & writeNameAndQuotedValue inside the if statment.

> +    // FIXME: Add the other SVGResources here.
Same comment change needed.

> Index: WebCore/svg/SVGMaskElement.cpp

> -        invalidateCanvasResources();
> +        if (RenderSVGResource* resource = renderer()->toRenderSVGResource())
> +            resource->invalidateClients();
>  }

> -    invalidateCanvasResources();
> +
> +    if (!renderer())
> +        return;
> +
> +    if (RenderSVGResource* resource = renderer()->toRenderSVGResource())
> +        resource->invalidateClients();
>  }

Why not leave the invalidateCanvasResource() function, and move your code querying toRenderSVGResource() there?
I'd even propose to move that code into SVGStyledElement::invalidateCanvasResources(), as it's resource-type agnostic, and will work for clippers etc.. as well.


> Index: WebCore/svg/SVGStyledElement.cpp
> -        if (SVGStyledElement* styledElement = static_cast<SVGStyledElement*>(element->isStyled() ? element : 0))
> +        if (SVGStyledElement* styledElement = static_cast<SVGStyledElement*>(element->isStyled() ? element : 0)) {
> +            RenderObject* renderer = styledElement->renderer();
> +            if (renderer && renderer->isSVGResource())
> +                renderer->toRenderSVGResource()->invalidateClients();
>              styledElement->invalidateCanvasResources();
> +        }

If you follow my suggestion from above, moving the toRenderSVGResource()->invalidateclients() stuff into invalidateCanvasResources, then you can save the change above as well.

> Index: WebCore/svg/SVGUnitTypes.h
> ===================================================================
> --- WebCore/svg/SVGUnitTypes.h	(revision 54874)
> +++ WebCore/svg/SVGUnitTypes.h	(working copy)
> @@ -27,6 +27,7 @@
>  namespace WebCore {
>  
>  class SVGUnitTypes : public RefCounted<SVGUnitTypes> {
> +
>  public:

Newline too much.

> +static inline SVGUnitTypes::SVGUnitType toUnitType(int input) { return static_cast<SVGUnitTypes::SVGUnitType>(input); }
s/input/type/

>  } // namespace WebCore
>  
>  #endif // ENABLE(SVG)

Remmove the visual noise.

Ok, still some issues, even after the 115th iteration ;-) But the concept is sound, the code is fine, just some nitpicking :-)
Happy to review the next iteration..

[Dirk Schulze]

Created attachment 48911 [details]
Patch

[WebKit Review Bot]

Attachment 48911 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.h': Can't open for reading
WebCore/rendering/RenderSVGResource.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.cpp': Can't open for reading
WebCore/rendering/RenderSVGResourceMasker.h:27:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 48911 [details]
Patch

I'm tempted to r+ it, though still some things to be changed: most 
> Index: WebCore/Android.mk
> +    rendering/RenderSVGResourceMasker.cpp \

Tab still there.

> Index: WebCore/rendering/SVGRenderTreeAsText.cpp
> +    switch (unitType) {
> +    case SVGUnitTypes::SVG_UNIT_TYPE_UNKNOWN :
> +    case SVGUnitTypes::SVG_UNIT_TYPE_USERSPACEONUSE :
> +    case SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX :

There's a trailing space after the identifier, please remove.

> +void writeResourcesToObject(TextStream& ts, const RenderObject& object, int indent)
writeReosurcesToObject?? That naming is quite confusing. I'd vote for just writeResources().
There's already writeRenderResources, that will die soon.

> Index: WebCore/svg/SVGMaskElement.cpp
> -        invalidateCanvasResources();
> +        SVGStyledElement::invalidateCanvasResources();

You can omit SVGStyledElement:: here.

> -    invalidateCanvasResources();
> +    SVGStyledElement::invalidateCanvasResources();

Ditto.

Can you please also fix the two include issues reported by the style bot?

I'd r+ it, though we need to upload a new patch anyways (for possible roll-outs etc.., you'll never know...)



> Index: WebCore/svg/SVGStyledElement.cpp
> -        if (SVGStyledElement* styledElement = static_cast<SVGStyledElement*>(element->isStyled() ? element : 0))
> +        if (SVGStyledElement* styledElement = static_cast<SVGStyledElement*>(element->isStyled() ? element : 0)) 

Please remove the trailing whitespace.


>  void SVGStyledElement::invalidateCanvasResources()
>  {
> -    if (SVGResource* resource = canvasResource(renderer()))
> +    RenderObject* object = renderer();

Please ASSERT(object); here.

> +    if (object->isSVGResource())
> +        object->toRenderSVGResource()->invalidateClients();
> +

Please add a comment here, stating that the lines below will be removed soon.

> +    if (SVGResource* resource = canvasResource(object))
>          resource->invalidate();

Ah and I think no one else is overriding invalidatecanvasResources(), so you can de-virtualize it, see SVGStyledElement.h

[Dirk Schulze]

Created attachment 48930 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 48930 [details]
Patch

excellent, r=me.

[Eric Seidel (no email)]

Attachment 48930 [details] was posted by a committer and has review+, assigning to Dirk Schulze for commit.

[Dirk Schulze]

Created attachment 49043 [details]
Patch

[WebKit Review Bot]

Attachment 49043 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.h': Can't open for reading
Skipping input 'WebCore/svg/graphics/SVGResourceMasker.cpp': Can't open for reading
WebCore/rendering/SVGRenderTreeAsText.h:64:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 49043 [details]
Patch

[maskUnits=objectBoundingBox] [maskContentUnits=objectBoundingBox]

Should we be printing the default values?

It's sad that we have to call this everywhere:
deregisterFromResources

I'm not really sure why that's required of *all* svg render objects.

[Eric Seidel (no email)]

 void SVGRenderBase::deregisterFromResources(RenderObject* object)
 306 {
 307     // We only have a renderer for masker at the moment.
 308     if (RenderSVGResourceMasker* resource = getRenderSVGResourceById<RenderSVGResourceMasker>(object->document(), object->style()->svgStyle()->maskElement()))
 309         resource->invalidateClient(object);
 310 }

I guess I understand now.  Still slightly unfortunate.

[Eric Seidel (no email)]

We don't normally add commented out code:
 64 //TextStream& operator<<(TextStream& ts, const SVGUnitTypes&);

[Eric Seidel (no email)]

Why remove the comments?  I think they're helpful:

 } // namespace WebCore
 68 }
8069 
81  #endif // ENABLE(SVG)
 70 #endif

[Eric Seidel (no email)]

Comment on attachment 49043 [details]
Patch

r- for above nits.

[Nikolas Zimmermann]

(In reply to comment #17)
> Why remove the comments?  I think they're helpful:
> 
>  } // namespace WebCore
>  68 }
> 8069 
> 81  #endif // ENABLE(SVG)
>  70 #endif

I think they're not, and asked Dirk to remove them. I call it visual noise, and I think that's the outcome of the discussion on webkit-dev some months ago.

We're not using nested namespaces on a regular base, if we'd do these comments have some value, for other files... it's noise.

[Nikolas Zimmermann]

(In reply to comment #14)
> (From update of attachment 49043 [details])
> [maskUnits=objectBoundingBox] [maskContentUnits=objectBoundingBox]
> 
> Should we be printing the default values?
I think so, you can not be sure the SVG DOM default values are passed correctly into the renderer.
Always dumping values reflects the state in the RenderSVGResource classes and is a benefit, when debugging and does not have a lot of cost. In fact we should dump a lot more SVG specific information in the -expected.txt files, it's so cheap and helps to track regressions, etc :-)

[Dirk Schulze]

Created attachment 49045 [details]
Patch

fixed the style issue and delete the line.
I don't changed the deletion of the comments, according to Nikos suggestion and the discussion on #webkit-dev.

I think it is uesful to dump the unittypes of a masker and we do it only on the resource.

[Nikolas Zimmermann]

Comment on attachment 49045 [details]
Patch

The commented code is gone, and I think it's safe to r+ now, as the invalidateCanvasResources() ASSERT() is gone, leading to the world-breakage when landing this patch the last time.
Calling deregisterFromResources() is cheap, it's only done on destruction of renderers. This is not a common case ;-)

[Eric Seidel (no email)]

(In reply to comment #19)
> I think they're not, and asked Dirk to remove them. I call it visual noise, and
> I think that's the outcome of the discussion on webkit-dev some months ago.
> 
> We're not using nested namespaces on a regular base, if we'd do these comments
> have some value, for other files... it's noise.

Could you cite the discussion?  If this is a style difference, it should be added to the guidelines and checked by the check-webkit-style script and the bot.

[Eric Seidel (no email)]

(In reply to comment #22)
> (From update of attachment 49045 [details])
> The commented code is gone, and I think it's safe to r+ now, as the
> invalidateCanvasResources() ASSERT() is gone, leading to the world-breakage
> when landing this patch the last time.
> Calling deregisterFromResources() is cheap, it's only done on destruction of
> renderers. This is not a common case ;-)

It's not about how expensive it is, it's about how many places in the code it's called from.  But I think it's OK for now.

[Nikolas Zimmermann]

(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 49045 [details] [details])
> > The commented code is gone, and I think it's safe to r+ now, as the
> > invalidateCanvasResources() ASSERT() is gone, leading to the world-breakage
> > when landing this patch the last time.
> > Calling deregisterFromResources() is cheap, it's only done on destruction of
> > renderers. This is not a common case ;-)
> 
> It's not about how expensive it is, it's about how many places in the code it's
> called from.  But I think it's OK for now.

Oh absolutely. Dirk already minimized it - in an ideal world only RenderSVGModelObject & RenderSVGRoot would need it. All other calls are needed because there's no common base :(
(I thought you already considered that)

[Eric Seidel (no email)]

According to the current style guide, the // namespace WebCore comments are recommended:
https://webkit.org/coding/coding-style.html

See numerous examples in that file.

Clearly we need to add a check-webkit-style checker for such.  I'll file a bug.

[Eric Seidel (no email)]

Filed bug 35133.

[Nikolas Zimmermann]

(In reply to comment #26)
> According to the current style guide, the // namespace WebCore comments are
> recommended:
> https://webkit.org/coding/coding-style.html
> 
> See numerous examples in that file.
> 
> Clearly we need to add a check-webkit-style checker for such.  I'll file a bug.

Yeah, please file a bug to remove the rule as well. No idea what happened with the discussion, but there are people sharing my opinion: http://article.gmane.org/gmane.os.opendarwin.webkit.devel/10563/match=visual+noise

[Eric Seidel (no email)]

Comment on attachment 49045 [details]
Patch

The copyrights are wrong on these files too.  They only say RIM, and yet clearly have lots of copied code.

[Eric Seidel (no email)]

(In reply to comment #20)
> (In reply to comment #14)
> > (From update of attachment 49043 [details] [details])
> > [maskUnits=objectBoundingBox] [maskContentUnits=objectBoundingBox]
> > 
> > Should we be printing the default values?
> I think so, you can not be sure the SVG DOM default values are passed correctly
> into the renderer.
> Always dumping values reflects the state in the RenderSVGResource classes and
> is a benefit, when debugging and does not have a lot of cost. In fact we should
> dump a lot more SVG specific information in the -expected.txt files, it's so
> cheap and helps to track regressions, etc :-)

I'm OK with always dumping, but this is a change from historical policy of the rendering tree dumps.

At least that was my impression of "policy" when I wrote the rendering tree dumps for SVG, and is still my understanding of HTML's approach.  However the days are long-gone where rendering tree dumps are particularly human readable (which I think was the impetus behind dumping only the non-default state).

[Dirk Schulze]

(In reply to comment #29)
> (From update of attachment 49045 [details])
> The copyrights are wrong on these files too.  They only say RIM, and yet
> clearly have lots of copied code.

The relevant code, that I copied, was from me. See bug reports:

https://bugs.webkit.org/show_bug.cgi?id=19243
https://bugs.webkit.org/show_bug.cgi?id=30480
https://bugs.webkit.org/show_bug.cgi?id=31980
https://bugs.webkit.org/show_bug.cgi?id=32738

and others.

I don't care if  my name is on top of RenderSVGResourceMasker. The commit log still shows where the code comes from.

[Dirk Schulze]

Landed patch in r54991

[Dirk Schulze]

Comment on attachment 49045 [details]
Patch

clearing review flag. Patch was landed in r54991.

[Dirk Schulze]

(In reply to comment #30)
> 
> I'm OK with always dumping, but this is a change from historical policy of the
> rendering tree dumps.
> 
> At least that was my impression of "policy" when I wrote the rendering tree
> dumps for SVG, and is still my understanding of HTML's approach.  However the
> days are long-gone where rendering tree dumps are particularly human readable
> (which I think was the impetus behind dumping only the non-default state).

The dumps ar more useful now. I think they are still readable. It's a oneliner, where you see the id, resource type and unit types. And I think that the information are still useful. Some people may don't know the default values or don't trust WebKit to use them.

[Dirk Schulze]

closing bug, all patches got landed.