Bug 94419 – Clean up SVGLocatable inheritance

• Component: SVG
• Status: RESOLVED FIXED

[Philip Rogers]

SVGLocatable's inheritance chain is a little crufty. SVGTransformableElement inherits from SVGLocatableElement but unnecessarily re-defines the SVGLocatable functions. Furthermore, SVGPathElement defines getBBox() but the function is deadcode.

[Philip Rogers]

Created attachment 159276 [details]
Cleanup SVGLocatable inheritance

[Philip Rogers]

Reviewers: for context, here's an incomplete but useful description of the inheritance tree:
https://docs.google.com/a/chromium.org/drawings/d/1h9TLoPbMI6h6q_RZK3BNgVc0nhxkBWStkpTYYozzeWU/edit

[Build Bot]

Comment on attachment 159276 [details]
Cleanup SVGLocatable inheritance

Attachment 159276 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13529452

[Philip Rogers]

The windows build has a warning that is causing the build to fail. This is real and might be why the code was architected this way to begin with.
http://msdn.microsoft.com/en-us/library/6b3sy7ae(v=vs.80).aspx

The issue is we have a diamond inheritance chain:
SVGLocatable -> SVGTransformable -> SVGStyledTransformableElement
SVGLocatable -> SVGStyledLocatable -> SVGStyledLocatableElement -> SVGStyledTransformableElement
getBBox() is only defined on SVGStyledLocatableElement but the win build is complaining that SVGStyledTransformableElement.getBBox() doesn't know which of the two parents to use.

Removing the review flag for now.

[Dirk Schulze]

(In reply to comment #4)
> The windows build has a warning that is causing the build to fail. This is real and might be why the code was architected this way to begin with.
> http://msdn.microsoft.com/en-us/library/6b3sy7ae(v=vs.80).aspx
> 
> The issue is we have a diamond inheritance chain:
> SVGLocatable -> SVGTransformable -> SVGStyledTransformableElement
> SVGLocatable -> SVGStyledLocatable -> SVGStyledLocatableElement -> SVGStyledTransformableElement
> getBBox() is only defined on SVGStyledLocatableElement but the win build is complaining that SVGStyledTransformableElement.getBBox() doesn't know which of the two parents to use.
> 
> Removing the review flag for now.

Yes, we had this issue before that prevent us from cleaning up in the past. However, like said before, we will have a major refactoring later anyway. Since SVG 2 will switch to WebIDL, multiple inheritances are not allowed anymore. Don't spend to  much time on it yet.

[Philip Rogers]

(In reply to comment #5)
> (In reply to comment #4)
> > The windows build has a warning that is causing the build to fail. This is real and might be why the code was architected this way to begin with.
> > http://msdn.microsoft.com/en-us/library/6b3sy7ae(v=vs.80).aspx
> > 
> > The issue is we have a diamond inheritance chain:
> > SVGLocatable -> SVGTransformable -> SVGStyledTransformableElement
> > SVGLocatable -> SVGStyledLocatable -> SVGStyledLocatableElement -> SVGStyledTransformableElement
> > getBBox() is only defined on SVGStyledLocatableElement but the win build is complaining that SVGStyledTransformableElement.getBBox() doesn't know which of the two parents to use.
> > 
> > Removing the review flag for now.
> 
> Yes, we had this issue before that prevent us from cleaning up in the past. However, like said before, we will have a major refactoring later anyway. Since SVG 2 will switch to WebIDL, multiple inheritances are not allowed anymore. Don't spend to  much time on it yet.

Sounds good to me; I'm not a fan of multiple inheritance. Want me to extract out the deadcode removal in SVGPathElement and land that? It's genuinely wrong and confusing for anyone looking at bounding box code.

[Philip Rogers]

Created attachment 159333 [details]
Remove incorrect getBBox() code

Extracting out the useful piece from the previous patch.

[Dirk Schulze]

Comment on attachment 159333 [details]
Remove incorrect getBBox() code

View in context: https://bugs.webkit.org/attachment.cgi?id=159333&action=review

r=me, but...

> Source/WebCore/ChangeLog:9
> +        SVGPathElement defines a getBBox() function that is both wrong and
> +        deadcode. This patch cleans this up.

I would agree that we should return objectBoundingBox(), but is does not seem to be "deadcode". Can you change this comment please?

[Philip Rogers]

Created attachment 159451 [details]
Update comment.

@krit, good catch.

[WebKit Review Bot]

Comment on attachment 159451 [details]
Update comment.

Clearing flags on attachment: 159451

Committed r126056: <http://trac.webkit.org/changeset/126056>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

This change broke svg/custom/getBBox-path.svg on platform mac.  See https://bugs.webkit.org/show_bug.cgi?id=94969.