Bug 35020 - Move SVGResources to Renderers, starting with Masker
: Move SVGResources to Renderers, starting with Masker
Status: RESOLVED FIXED
: WebKit
SVG
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-02-17 00:00 PST by
Modified: 2010-02-18 23:44 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-02-17 00:00:15 PST
Created an attachment (id=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 From 2010-02-17 00:03:47 PST -------
(From update of attachment 48866 [details])
Need to practice with webkit-patch a bit more :-P
------- Comment #2 From 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 From 2010-02-17 00:27:02 PST -------
Created an attachment (id=48867) [details]
Patch
------- Comment #4 From 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 From 2010-02-17 06:53:23 PST -------
(From update of attachment 48867 [details])
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 From 2010-02-17 10:44:32 PST -------
Created an attachment (id=48911) [details]
Patch
------- Comment #7 From 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 From 2010-02-17 12:21:17 PST -------
(From update of attachment 48911 [details])
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 From 2010-02-17 13:18:59 PST -------
Created an attachment (id=48930) [details]
Patch
------- Comment #10 From 2010-02-17 13:39:32 PST -------
(From update of attachment 48930 [details])
excellent, r=me.
------- Comment #11 From 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 From 2010-02-18 14:34:42 PST -------
Created an attachment (id=49043) [details]
Patch
------- Comment #13 From 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 From 2010-02-18 14:43:38 PST -------
(From update of attachment 49043 [details])
[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 From 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 From 2010-02-18 14:46:00 PST -------
We don't normally add commented out code:
 64 //TextStream& operator<<(TextStream& ts, const SVGUnitTypes&);
------- Comment #17 From 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 From 2010-02-18 14:47:05 PST -------
(From update of attachment 49043 [details])
r- for above nits.
------- Comment #19 From 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 From 2010-02-18 14:54:51 PST -------
(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 :-)
------- Comment #21 From 2010-02-18 14:58:39 PST -------
Created an attachment (id=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 From 2010-02-18 15:04:46 PST -------
(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 ;-)
------- Comment #23 From 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 From 2010-02-18 15:18:00 PST -------
(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.
------- Comment #25 From 2010-02-18 15:22:12 PST -------
(In reply to comment #24)
> (In reply to comment #22)
> > (From update of attachment 49045 [details] [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 From 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 From 2010-02-18 15:26:09 PST -------
Filed bug 35133.
------- Comment #28 From 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 From 2010-02-18 15:27:22 PST -------
(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.
------- Comment #30 From 2010-02-18 15:30:13 PST -------
(In reply to comment #20)
> (In reply to comment #14)
> > (From update of attachment 49043 [details] [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 From 2010-02-18 15:50:11 PST -------
(In reply to comment #29)
> (From update of attachment 49045 [details] [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 From 2010-02-18 15:50:47 PST -------
Landed patch in r54991
------- Comment #33 From 2010-02-18 15:51:50 PST -------
(From update of attachment 49045 [details])
clearing review flag. Patch was landed in r54991.
------- Comment #34 From 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 From 2010-02-18 23:44:10 PST -------
closing bug, all patches got landed.