Bug 15528 – svg_dynamic_cast should be removed

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

svg_dynamic_cast should be removed

svg_dynamic_cast is a hold-over crutch from the KSVG2 days where KSVG2 depended on dynamic_cast.  It's old-style code, which doesn't really fit with the current style for the rest of webkit.  We should remove this crutch and change over all the places in the code to just check node->isSVGElement() and static_cast themselves.

[Rob Buis]

Created attachment 17780 [details]
First attempt

The patch should be clear :) One use of svg_dynamic_cast was not needed at all. The spots that need extra attention are in SVGRootInlineBox.cpp and SVGAnimationElement.cpp.
Cheers,

Rob.

[Darin Adler]

Comment on attachment 17780 [details]
First attempt

 918                 if (node && node->isSVGElement())
 919                     textContent = static_cast<SVGTextContentElement*>(node);

Why is isSVGElement() sufficient evidence that this is a SVGTextContentElement? What guarantees that?

 83             target = ownerDocument()->getElementById(SVGURIReference::getTarget(m_href));

Should just use document() here instead of ownerDocument(). ownerDocument() is a lsower was to do the same thing.

 85             target = parentNode();
8686             while (target) {
8787                 if (target->nodeType() != ELEMENT_NODE)
8888                     target = target->parentNode();
8989                 else
9090                     break;
9191             }

This code seems unneeded to me. What node has a parent that's not an element?

r=me

[Rob Buis]

Hi Darin,

(In reply to comment #2)
> (From update of attachment 17780 [details] [edit])
>  918                 if (node && node->isSVGElement())
>  919                     textContent =
> static_cast<SVGTextContentElement*>(node);
> 
> Why is isSVGElement() sufficient evidence that this is a SVGTextContentElement?
> What guarantees that?

It is not. Just that the old code basically did the same thing, only with an extra assert and static_cast AFAICS.

>  83             target =
> ownerDocument()->getElementById(SVGURIReference::getTarget(m_href));
> 
> Should just use document() here instead of ownerDocument(). ownerDocument() is
> a lsower was to do the same thing.

Ok, good to know, will fix.

>  85             target = parentNode();
> 8686             while (target) {
> 8787                 if (target->nodeType() != ELEMENT_NODE)
> 8888                     target = target->parentNode();
> 8989                 else
> 9090                     break;
> 9191             }
> 
> This code seems unneeded to me. What node has a parent that's not an element?

Maybe a document(I didnt check so far)? I didnt want to change the existing code much, except that obvious SVGAnimationElement cleanup.

> r=me

Thx! Will land after some sleep :)
Mfg,

Rob.

[Rob Buis]

Landed in r28559.