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.
Created attachment 159276 [details] Cleanup SVGLocatable inheritance
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
Comment on attachment 159276 [details] Cleanup SVGLocatable inheritance Attachment 159276 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13529452
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.
(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.
(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.
Created attachment 159333 [details] Remove incorrect getBBox() code Extracting out the useful piece from the previous patch.
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?
Created attachment 159451 [details] Update comment. @krit, good catch.
Comment on attachment 159451 [details] Update comment. Clearing flags on attachment: 159451 Committed r126056: <http://trac.webkit.org/changeset/126056>
All reviewed patches have been landed. Closing bug.
This change broke svg/custom/getBBox-path.svg on platform mac. See https://bugs.webkit.org/show_bug.cgi?id=94969.