Summary: | svg_dynamic_cast should be removed | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | Keywords: | EasyFix | ||||
Priority: | P2 | ||||||
Version: | 523.x (Safari 3) | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.4 | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2007-10-15 22:02:26 PDT
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 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
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. |