Bug 137137 – Stop using legacy NODE_TYPE_CASTS() macro for HTML Elements

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Stop using legacy NODE_TYPE_CASTS() macro for HTML Elements and use the new SPECIALIZE_TYPE_TRAITS_*() macro instead so that is<>() / downcast<>() works for those types.

[Chris Dumez]

Created attachment 238695 [details]
WIP Patch

[WebKit Commit Bot]

Attachment 238695 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 94 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 238696 [details]
Patch

[WebKit Commit Bot]

Attachment 238696 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 238697 [details]
Patch

[WebKit Commit Bot]

Attachment 238697 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 238714 [details]
Patch

[WebKit Commit Bot]

Attachment 238714 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 238714 [details]
Patch

Attachment 238714 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4672407436525568

New failing tests:
editing/unsupported-content/list-type-before.html
editing/style/push-down-font-styles-win.html
editing/style/fore-color-by-name.html
editing/pasteboard/4744008.html
editing/unsupported-content/table-type-before.html
editing/unsupported-content/list-delete-003.html
editing/unsupported-content/list-delete-001.html
editing/unsupported-content/list-type-after.html
editing/unsupported-content/table-delete-002.html
editing/style/push-down-font-styles-mac.html
editing/unsupported-content/table-type-after.html
editing/pasteboard/insert-font-with-size-and-css.html
editing/style/inline-style-container.html
editing/style/inline-style-extend-run.html

[Build Bot]

Created attachment 238720 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 238714 [details]
Patch

Attachment 238714 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6201980582100992

New failing tests:
editing/style/push-down-font-styles-win.html
editing/style/fore-color-by-name.html
editing/pasteboard/4744008.html
editing/style/push-down-font-styles-mac.html
editing/pasteboard/insert-font-with-size-and-css.html
editing/style/inline-style-container.html
editing/style/inline-style-extend-run.html

[Build Bot]

Created attachment 238721 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 238731 [details]
Patch

[WebKit Commit Bot]

Attachment 238731 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 238731 [details]
Patch

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()->firstChild());

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

[Chris Dumez]

Comment on attachment 238731 [details]
Patch

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

>> Source/WebCore/bindings/js/JSPluginElementFunctions.cpp:130
>> +        return false;
> 
> Uh!

Yes, the function returns a boolean.

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

Ok.

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

Ok.

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

Ok.

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

Ok.

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

Ok.

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

Ok.

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

lol, Ok.

>> Source/WebCore/html/shadow/MediaControlElementTypes.cpp:64
>> +    HTMLElement* element = downcast<HTMLElement>(node);
> 
> Hum, this looks fragile.

Ok. I'll remove the cast.

>> 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)

Ok.

>> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:520
>> +        if ((is<HTMLFrameElementBase>(node) || is<HTMLObjectElement>(node))
> 
> Isn't this is<HTMLFrameOwnerElement>(node)?

No, HTMLFrameOwnerElement also covers HTMLAppletElement and HTMLEmbedElement.

>> Source/WebCore/mathml/MathMLElement.cpp:87
>> +        return true;
> 
> This isn't strictly equivalent.
> 
> Previously, the remaining of the function would not be evaluated if node is a mathMLElement.

Ok, good catch.

>> Source/WebCore/mathml/MathMLElement.cpp:90
>> +        return true;
> 
> ditto.

Ok.

>> Source/WebCore/mathml/MathMLElement.cpp:276
>>          }
> 
> etc

Ok.

[Chris Dumez]

Created attachment 238744 [details]
Patch

[WebKit Commit Bot]

Attachment 238744 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 238744 [details]
Patch

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

> Source/WebCore/page/Frame.cpp:397
>          if (n->hasTagName(tdTag) && !startingTableCell) {
> -            startingTableCell = toHTMLTableCellElement(n);
> +            startingTableCell = downcast<HTMLTableCellElement>(n);
>          } else if (n->hasTagName(trTag) && startingTableCell) {

Maybe take the opportunity to fix the style here?

> Source/WebCore/page/SpatialNavigation.cpp:-762
> -};

Weird, I thought the compiler had a warning for this.

[Chris Dumez]

Created attachment 238752 [details]
Patch

[WebKit Commit Bot]

Attachment 238752 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLTableCellElement.h:67:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTableSectionElement.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLMediaElement.h:915:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLTextFormControlElement.h:156:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLPlugInImageElement.h:162:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/HTMLFrameElementBase.h:81:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 97 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 238752 [details]
Patch

Clearing flags on attachment: 238752

Committed r174031: <http://trac.webkit.org/changeset/174031>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.