[webkit-reviews] review granted: [Bug 137004] Use downcast<SVG*Element>() instead of toSVG*Element() : [Attachment 238486] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137004: Use downcast<SVG*Element>() instead of toSVG*Element()
https://bugs.webkit.org/show_bug.cgi?id=137004

Attachment 238486: Patch
https://bugs.webkit.org/attachment.cgi?id=238486&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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