Bug 137068 – Use is<HTML*Element>() instead of isHTML*Element() - Part 1

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Start using is<HTML*Element>() instead of isHTML*Element().

[Chris Dumez]

Created attachment 238600 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 238600 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:595
> -        if (!isHTMLFormControlElement(element) && !isHTMLObjectElement(element))
> +        if (!isHTMLFormControlElement(element) && !is<HTMLObjectElement>(element))

Why don't we use is<HTMLFormControlElement> since we're modifying this line anyway?

> Source/WebCore/html/HTMLNameCollection.cpp:70
> +    return element.hasTagName(appletTag) || (is<HTMLObjectElement>(element) && downcast<HTMLObjectElement>(element).isDocNamedItem())
>          || (isHTMLImageElement(element) && element.hasName());

Why don't we also use is<HTMLImageElement> while we're at it?

> Source/WebCore/html/HTMLNameCollection.cpp:77
> +        || element.hasTagName(appletTag) || (is<HTMLObjectElement>(element) && downcast<HTMLObjectElement>(element).isDocNamedItem())
>          || isHTMLImageElement(element);

Ditto.

> Source/WebCore/html/HTMLSelectElement.cpp:366
> -        return isHTMLOptionElement(child) || isHTMLOptGroupElement(child) || validationMessageShadowTreeContains(child);
> +        return is<HTMLOptionElement>(child) || isHTMLOptGroupElement(child) || validationMessageShadowTreeContains(child);

Ditto about using is<HTMLOptGroupElement>.

> Source/WebCore/html/parser/HTMLElementStack.cpp:123
>      return !isHTMLOptGroupElement(item->node()) && !isHTMLOptionElement(item->node());
> +    return !isHTMLOptGroupElement(item->node()) && !is<HTMLOptionElement>(item->node());

Ditto about using is<HTMLOptGroupElement>.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:519
>          if ((isHTMLFrameElement(node) || isHTMLIFrameElement(node) || isHTMLObjectElement(node))
> +        if ((isHTMLFrameElement(node) || isHTMLIFrameElement(node) || is<HTMLObjectElement>(node))

Ditto for isHTMLFrameElement and isHTMLIFrameElement.

> Source/WebCore/rendering/HitTestResult.cpp:321
>          || isHTMLImageElement(m_innerNonSharedNode.get())

Why don't we also use is<HTMLImageElement>?

> Source/WebCore/rendering/RenderBox.cpp:2480
> +    if (logicalWidth.type() == Auto && !isStretchingColumnFlexItem(*this) && element() && (is<HTMLInputElement>(element()) || element()->hasTagName(selectTag) || element()->hasTagName(buttonTag) || is<HTMLTextAreaElement>(element()) || element()->hasTagName(legendTag)))

It's weird that we don't use is<~> for only two types of elements. Can we fix that?

> Source/WebCore/rendering/RenderFileUploadControl.cpp:258
> +    return buttonNode && buttonNode->isHTMLElement() && is<HTMLInputElement>(buttonNode) ? downcast<HTMLInputElement>(buttonNode) : nullptr;

Isn't it redundant to check buttonNode->isHTMLElement()?

[Chris Dumez]

Comment on attachment 238600 [details]
Patch

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

Thanks

>> Source/WebCore/html/HTMLFormElement.cpp:595
>> +        if (!isHTMLFormControlElement(element) && !is<HTMLObjectElement>(element))
> 
> Why don't we use is<HTMLFormControlElement> since we're modifying this line anyway?

is<HTMLFormControlElement>() does not work yet. It needs https://bugs.webkit.org/show_bug.cgi?id=137056 to land first.

>> Source/WebCore/html/HTMLNameCollection.cpp:70
>>          || (isHTMLImageElement(element) && element.hasName());
> 
> Why don't we also use is<HTMLImageElement> while we're at it?

Sure, I can update the others on the lines I edited in this patch. Those classes are in my Part 2 anyway.

>> Source/WebCore/rendering/RenderBox.cpp:2480
>> +    if (logicalWidth.type() == Auto && !isStretchingColumnFlexItem(*this) && element() && (is<HTMLInputElement>(element()) || element()->hasTagName(selectTag) || element()->hasTagName(buttonTag) || is<HTMLTextAreaElement>(element()) || element()->hasTagName(legendTag)))
> 
> It's weird that we don't use is<~> for only two types of elements. Can we fix that?

Yes, once I am done with the transition, I am planning to use is<>() more instead of hasTagName() more in a follow-up patch. However, I'll take care of this line in this patch.

>> Source/WebCore/rendering/RenderFileUploadControl.cpp:258
>> +    return buttonNode && buttonNode->isHTMLElement() && is<HTMLInputElement>(buttonNode) ? downcast<HTMLInputElement>(buttonNode) : nullptr;
> 
> Isn't it redundant to check buttonNode->isHTMLElement()?

It is, I'll drop it.

[Chris Dumez]

Created attachment 238608 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238608 [details]
Patch

Clearing flags on attachment: 238608

Committed r173932: <http://trac.webkit.org/changeset/173932>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

This broke my WinCairo build.  is is not defined in DomCoreClasses.cpp.  https://www.youtube.com/watch?v=j4XT-l-_3y0

[Chris Dumez]

(In reply to comment #7)
> This broke my WinCairo build.  is is not defined in DomCoreClasses.cpp.  https://www.youtube.com/watch?v=j4XT-l-_3y0

Looks like we need "WebCore::" Apparently this code is not under WebCore namespace?

[Chris Dumez]

(In reply to comment #7)
> This broke my WinCairo build.  is is not defined in DomCoreClasses.cpp.  https://www.youtube.com/watch?v=j4XT-l-_3y0

Build fix landed in http://trac.webkit.org/changeset/173938. Thanks for notifying me.