[webkit-reviews] review denied: [Bug 137137] Stop using legacy NODE_TYPE_CASTS() macro for HTML Elements : [Attachment 238731] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137137: Stop using legacy NODE_TYPE_CASTS() macro for HTML Elements
https://bugs.webkit.org/show_bug.cgi?id=137137

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=238731&action=review


> Source/WebCore/bindings/js/JSPluginElementFunctions.cpp:130
> -	   return 0;
> +	   return false;

Uh!

> Source/WebCore/css/StyleResolver.cpp:454
>	   return 0;

nullptr!

> Source/WebCore/css/StyleResolver.cpp:456
>	   return 0;

nullptr!

> Source/WebCore/html/HTMLElement.cpp:871
> +    return is<HTMLElement>(node) &&
(downcast<HTMLElement>(node).hasTagName(bdiTag) ||
downcast<HTMLElement>(node).fastHasAttribute(dirAttr));

Why not is<HTMLBDIElement>?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:-419
> -	   if (node->isPluginElement()) {
> -	       HTMLPlugInElement* plugInElement = toHTMLPlugInElement(node);
> -	       if (plugInElement->isPlugInImageElement()) {

This code was weird.

> Source/WebCore/html/HTMLSelectElement.cpp:452
> +	   before = downcast<HTMLElement>(options()->item(index+1));

index + 1

> Source/WebCore/html/HTMLSelectElement.cpp:474
> +	       add(downcast<HTMLElement>(option.get()), 0, ec);

0 -> nullptr

> Source/WebCore/html/LabelableElement.h:55
> +inline bool isLabelableElement(const Node& node) { return
node.isHTMLElement() && downcast<HTMLElement>(node).isLabelable(); }

node.isHTMLElement() -> is<HTMLElement>(node)

> Source/WebCore/html/RangeInputType.cpp:271
> -    return
&toHTMLElement(*element().userAgentShadowRoot()->firstChild()->firstChild());
> +    return
&downcast<HTMLElement>(*element().userAgentShadowRoot()->firstChild()->firstChi
ld());

Uh, what.

> Source/WebCore/html/shadow/MediaControlElementTypes.cpp:64
>      ASSERT_WITH_SECURITY_IMPLICATION(node->isMediaControlElement());
> -    HTMLElement* element = toHTMLElement(node);
> +    HTMLElement* element = downcast<HTMLElement>(node);

Hum, this looks fragile.

> Source/WebCore/html/shadow/TextControlInnerElements.h:74
> +inline bool isTextControlInnerTextElement(const Node& node) { return
node.isHTMLElement() &&
isTextControlInnerTextElement(downcast<HTMLElement>(node)); }

node.isHTMLElement() -> is<HTMLElement>(node)

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:520
> +	   if ((is<HTMLFrameElementBase>(node) || is<HTMLObjectElement>(node))

Isn't this is<HTMLFrameOwnerElement>(node)?

> Source/WebCore/mathml/MathMLElement.cpp:87
> -    if (node.isMathMLElement()) {
> -	   auto& mathmlElement = downcast<MathMLElement>(node);
> -	   return mathmlElement.hasTagName(MathMLNames::mathTag);
> -    }
> +    if (is<MathMLMathElement>(node))
> +	   return true;

This isn't strictly equivalent.

Previously, the remaining of the function would not be evaluated if node is a
mathMLElement.

> Source/WebCore/mathml/MathMLElement.cpp:90
> -    if (node.isSVGElement()) {
> -	   auto& svgElement = downcast<SVGElement>(node);
> -	   return svgElement.hasTagName(SVGNames::svgTag);
> -    }
> +    if (is<SVGSVGElement>(node))
> +	   return true;

ditto.

> Source/WebCore/mathml/MathMLElement.cpp:276
> -	   if (child.isMathMLElement() &&
(MathMLSelectElement::isMathMLEncoding(value) ||
MathMLSelectElement::isHTMLEncoding(value))) {
> -	       auto& mathmlElement = downcast<MathMLElement>(child);
> -	       return mathmlElement.hasTagName(MathMLNames::mathTag);
> -	   }
> +	   if (is<MathMLMathElement>(child) &&
(MathMLSelectElement::isMathMLEncoding(value) ||
MathMLSelectElement::isHTMLEncoding(value)))
> +	       return true;
>  
> -	   if (child.isSVGElement() &&
(MathMLSelectElement::isSVGEncoding(value) ||
MathMLSelectElement::isHTMLEncoding(value))) {
> -	       auto& svgElement = downcast<SVGElement>(child);
> -	       return svgElement.hasTagName(SVGNames::svgTag);
> -	   }
> +	   if (is<SVGSVGElement>(child) &&
(MathMLSelectElement::isSVGEncoding(value) ||
MathMLSelectElement::isHTMLEncoding(value)))
> +	       return true;
>  
> -	   if (child.isHTMLElement() &&
MathMLSelectElement::isHTMLEncoding(value)) {
> -	       auto& htmlElement = toHTMLElement(child);
> -	       return htmlElement.hasTagName(HTMLNames::htmlTag) ||
(isFlowContent(htmlElement) &&
StyledElement::childShouldCreateRenderer(child));
> +	   if (is<HTMLElement>(child) &&
MathMLSelectElement::isHTMLEncoding(value)) {
> +	       auto& htmlElement = downcast<HTMLElement>(child);
> +	       return is<HTMLHtmlElement>(htmlElement) ||
(isFlowContent(htmlElement) &&
StyledElement::childShouldCreateRenderer(child));
>	   }

etc