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.
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.
Landed in r28559.