Bug 15528 - svg_dynamic_cast should be removed
Summary: svg_dynamic_cast should be removed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords: EasyFix
Depends on:
Blocks:
 
Reported: 2007-10-15 22:02 PDT by Eric Seidel (no email)
Modified: 2007-12-08 00:54 PST (History)
0 users

See Also:


Attachments
First attempt (12.20 KB, patch)
2007-12-07 13:16 PST, Rob Buis
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-10-15 22:02:26 PDT
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.
Comment 1 Rob Buis 2007-12-07 13:16:19 PST
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.
Comment 2 Darin Adler 2007-12-07 13:55:48 PST
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
Comment 3 Rob Buis 2007-12-07 14:06:40 PST
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.
Comment 4 Rob Buis 2007-12-08 00:54:32 PST
Landed in r28559.