Bug 35020 - Move SVGResources to Renderers, starting with Masker
Summary: Move SVGResources to Renderers, starting with Masker
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-17 00:00 PST by Dirk Schulze
Modified: 2010-02-18 23:44 PST (History)
4 users (show)

See Also:


Attachments
Patch (154.39 KB, patch)
2010-02-17 00:00 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (161.87 KB, patch)
2010-02-17 00:27 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (162.48 KB, patch)
2010-02-17 10:44 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (161.76 KB, patch)
2010-02-17 13:18 PST, Dirk Schulze
zimmermann: review+
Details | Formatted Diff | Diff
Patch (160.79 KB, patch)
2010-02-18 14:34 PST, Dirk Schulze
eric: review-
Details | Formatted Diff | Diff
Patch (160.49 KB, patch)
2010-02-18 14:58 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2010-02-17 00:00:15 PST
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.
Comment 1 Dirk Schulze 2010-02-17 00:03:47 PST
Comment on attachment 48866 [details]
Patch

Need to practice with webkit-patch a bit more :-P
Comment 2 WebKit Review Bot 2010-02-17 00:05:12 PST
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.
Comment 3 Dirk Schulze 2010-02-17 00:27:02 PST
Created attachment 48867 [details]
Patch
Comment 4 WebKit Review Bot 2010-02-17 00:31:18 PST
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.
Comment 5 Nikolas Zimmermann 2010-02-17 06:53:23 PST
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..
Comment 6 Dirk Schulze 2010-02-17 10:44:32 PST
Created attachment 48911 [details]
Patch
Comment 7 WebKit Review Bot 2010-02-17 10:49:06 PST
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.
Comment 8 Nikolas Zimmermann 2010-02-17 12:21:17 PST
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
Comment 9 Dirk Schulze 2010-02-17 13:18:59 PST
Created attachment 48930 [details]
Patch
Comment 10 Nikolas Zimmermann 2010-02-17 13:39:32 PST
Comment on attachment 48930 [details]
Patch

excellent, r=me.
Comment 11 Eric Seidel (no email) 2010-02-17 14:23:56 PST
Attachment 48930 [details] was posted by a committer and has review+, assigning to Dirk Schulze for commit.
Comment 12 Dirk Schulze 2010-02-18 14:34:42 PST
Created attachment 49043 [details]
Patch
Comment 13 WebKit Review Bot 2010-02-18 14:36:06 PST
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.
Comment 14 Eric Seidel (no email) 2010-02-18 14:43:38 PST
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.
Comment 15 Eric Seidel (no email) 2010-02-18 14:45:28 PST
 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.
Comment 16 Eric Seidel (no email) 2010-02-18 14:46:00 PST
We don't normally add commented out code:
 64 //TextStream& operator<<(TextStream& ts, const SVGUnitTypes&);
Comment 17 Eric Seidel (no email) 2010-02-18 14:46:25 PST
Why remove the comments?  I think they're helpful:

 } // namespace WebCore
 68 }
8069 
81  #endif // ENABLE(SVG)
 70 #endif
Comment 18 Eric Seidel (no email) 2010-02-18 14:47:05 PST
Comment on attachment 49043 [details]
Patch

r- for above nits.
Comment 19 Nikolas Zimmermann 2010-02-18 14:50:55 PST
(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.
Comment 20 Nikolas Zimmermann 2010-02-18 14:54:51 PST
(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 :-)
Comment 21 Dirk Schulze 2010-02-18 14:58:39 PST
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.
Comment 22 Nikolas Zimmermann 2010-02-18 15:04:46 PST
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 ;-)
Comment 23 Eric Seidel (no email) 2010-02-18 15:16:40 PST
(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.
Comment 24 Eric Seidel (no email) 2010-02-18 15:18:00 PST
(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.
Comment 25 Nikolas Zimmermann 2010-02-18 15:22:12 PST
(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)
Comment 26 Eric Seidel (no email) 2010-02-18 15:23:01 PST
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.
Comment 27 Eric Seidel (no email) 2010-02-18 15:26:09 PST
Filed bug 35133.
Comment 28 Nikolas Zimmermann 2010-02-18 15:26:18 PST
(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
Comment 29 Eric Seidel (no email) 2010-02-18 15:27:22 PST
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.
Comment 30 Eric Seidel (no email) 2010-02-18 15:30:13 PST
(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).
Comment 31 Dirk Schulze 2010-02-18 15:50:11 PST
(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.
Comment 32 Dirk Schulze 2010-02-18 15:50:47 PST
Landed patch in r54991
Comment 33 Dirk Schulze 2010-02-18 15:51:50 PST
Comment on attachment 49045 [details]
Patch

clearing review flag. Patch was landed in r54991.
Comment 34 Dirk Schulze 2010-02-18 15:54:30 PST
(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.
Comment 35 Dirk Schulze 2010-02-18 23:44:10 PST
closing bug, all patches got landed.