Bug 137004 – Use downcast<SVG*Element>() instead of toSVG*Element()

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Use downcast<SVG*Element>() instead of toSVG*Element() and get rid of the transition macros for SVG Elements.

[Chris Dumez]

Created attachment 238484 [details]
Patch

[Chris Dumez]

Created attachment 238486 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238486&action=review

IMHO, you should review the use of pointers in the cases with:
    if (foo and foo->isBar())
        downcast<Bar>(foo)->bang();

Some of them use "downcast<Bar>(foo)->bang();", other do "downcast<Bar>(*foo).bang();". Unifying the style would be nice.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1503
> +        return downcast<SVGElement>(m_node)->title();

downcast<SVGElement>(*m_node).title()

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2886
> +    SVGSVGElement* rootElement = downcast<SVGDocument>(doc)->rootElement();

ditto.

> Source/WebCore/css/CSSCursorImageValue.cpp:53
>      return 0;

nullptr

> Source/WebCore/rendering/RenderTableCell.cpp:183
> -        // Nowrap is set, but we didn't actually use it because of the
> +        // Nowrap is set, but we didtoSVGImageElementn't actually use it because of the

hahahaha

> Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:99
> -        SVGGraphicsElement* styled = toSVGGraphicsElement(childNode);
> +        SVGGraphicsElement* styled = downcast<SVGGraphicsElement>(childNode);

Looks like this should be a ref.

> Source/WebCore/rendering/svg/RenderSVGResourceContainer.cpp:215
> -    SVGGraphicsElement* element = toSVGGraphicsElement(object->node());
> +    SVGGraphicsElement* element = downcast<SVGGraphicsElement>(object->node());

ditto.

> Source/WebCore/rendering/svg/RenderSVGTextPath.cpp:51
> -    SVGPathElement* pathElement = toSVGPathElement(targetElement);
> +    SVGPathElement* pathElement = downcast<SVGPathElement>(targetElement);

ditto.

> Source/WebCore/rendering/svg/RenderSVGTransformableContainer.cpp:44
>      SVGUseElement* useElement = 0;

nullptr

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:255
> +            if (SVGElement* element = child->node()->isSVGElement() ? downcast<SVGElement>(child->node()) : 0) {

nullptr

> Source/WebCore/rendering/svg/SVGRenderingContext.cpp:109
> +        SVGGraphicsElement& graphicsElement = *downcast<SVGGraphicsElement>(renderer.element());

I would put the dereference inside the downcast... not important though.

> Source/WebCore/rendering/svg/SVGTextLayoutEngine.cpp:429
> -    SVGElement* lengthContext = toSVGElement(text->parent()->element());
> +    SVGElement* lengthContext = downcast<SVGElement>(text->parent()->element());
>      
>      RenderObject* textParent = text->parent();
>      bool definesTextLength = textParent ? parentDefinesTextLength(textParent) : false;

There is something very very wrong going here, you should add a test.

text->parent() can be null apparently since it is checked by the ternary operator. BUT, it is used for lengthContext.

> Source/WebCore/svg/SVGDocument.cpp:48
>      return 0;

nullptr

> Source/WebCore/svg/SVGSVGElement.cpp:667
>              if (element->hasTagName(SVGNames::svgTag)) {

Can this be isSVGSVGElement()?

> Source/WebCore/svg/SVGTextContentElement.cpp:294
>          return 0;

nullptr

> Source/WebCore/svg/SVGTextContentElement.cpp:300
>          return 0;

nullptr

> Source/WebCore/svg/animation/SVGSMILElement.cpp:171
> +    SVGElement* svgTarget = target && target->isSVGElement() ? downcast<SVGElement>(target) : 0;

nullptr

[Darin Adler]

(In reply to comment #3)
> Some of them use "downcast<Bar>(foo)->bang();", other do "downcast<Bar>(*foo).bang();". Unifying the style would be nice.

downcast<Bar>(foo)->bang() -- prettier
downcast<Bar>(*foo).bang() -- clearer to the compiler when generating the code for downcast that foo can't be null, but modern compilers probably get this right already, since it knows that foo was dereferenced a couple lines earlier

I’ve been using downcast<Bar>(*foo).bang() because I never want to depend on the compiler to not add a bogus null check.

[Chris Dumez]

Created attachment 238500 [details]
Patch

[Chris Dumez]

Created attachment 238504 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238504 [details]
Patch

Clearing flags on attachment: 238504

Committed r173859: <http://trac.webkit.org/changeset/173859>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #7)
> (From update of attachment 238504 [details])
> Clearing flags on attachment: 238504
> 
> Committed r173859: <http://trac.webkit.org/changeset/173859>
It broke the debug builds, please fix.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 238504 [details] [details])
> > Clearing flags on attachment: 238504
> > 
> > Committed r173859: <http://trac.webkit.org/changeset/173859>
> It broke the debug builds, please fix.

Thanks for notifying me. I'll take a look now.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (From update of attachment 238504 [details] [details] [details])
> > > Clearing flags on attachment: 238504
> > > 
> > > Committed r173859: <http://trac.webkit.org/changeset/173859>
> > It broke the debug builds, please fix.
> 
> Thanks for notifying me. I'll take a look now.

http://trac.webkit.org/changeset/173866 should fix the build but I'll watch the bots to make sure.