Bug 137068

Summary: Use is<HTML*Element>() instead of isHTML*Element() - Part 1
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: alex.christensen, benjamin, commit-queue, darin, kling, koivisto, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 137015    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2014-09-24 09:25:11 PDT
Start using is<HTML*Element>() instead of isHTML*Element().
Comment 1 Chris Dumez 2014-09-24 09:30:49 PDT
Created attachment 238600 [details]
Patch
Comment 2 Ryosuke Niwa 2014-09-24 12:24:37 PDT
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()?
Comment 3 Chris Dumez 2014-09-24 12:28:46 PDT
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.
Comment 4 Chris Dumez 2014-09-24 13:40:06 PDT
Created attachment 238608 [details]
Patch
Comment 5 WebKit Commit Bot 2014-09-24 14:25:33 PDT
Comment on attachment 238608 [details]
Patch

Clearing flags on attachment: 238608

Committed r173932: <http://trac.webkit.org/changeset/173932>
Comment 6 WebKit Commit Bot 2014-09-24 14:25:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Alex Christensen 2014-09-24 16:22:12 PDT
This broke my WinCairo build.  is is not defined in DomCoreClasses.cpp.  https://www.youtube.com/watch?v=j4XT-l-_3y0
Comment 8 Chris Dumez 2014-09-24 16:24:49 PDT
(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?
Comment 9 Chris Dumez 2014-09-24 16:30:12 PDT
(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.