Bug 124645 – Use childrenOfType<> more in HTMLTableElement

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

[Gyuyoung Kim]

No need to use a loop to get first child in a tree. childrenOfType<> provides a functionality to find it easily.

[Gyuyoung Kim]

Created attachment 217406 [details]
Patch

[Gyuyoung Kim]

Darin, as you said https://bugs.webkit.org/show_bug.cgi?id=124571#c4, this patch simplified a loop to find a first children by using childrenOfType<>. However,  "childrenOfType<HTMLTableCaptionElement>(*this).first()" returns const element. So, I have to use const_cast to remove constness. Do you have better idea ?

[Build Bot]

Comment on attachment 217406 [details]
Patch

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

New failing tests:
dom/html/level2/html/HTMLCollection06.html
dom/xhtml/level2/html/HTMLTableElement21.xhtml
dom/html/level2/html/HTMLTableElement28.html
dom/html/level2/html/HTMLTableElement22.html
dom/html/level2/html/HTMLTableRowElement01.html
accessibility/table-one-cell.html
dom/html/level2/html/HTMLTableElement24.html
dom/html/level2/html/HTMLTableElement30.html
dom/html/level2/html/table07.html
dom/xhtml/level2/html/HTMLCollection07.xhtml
dom/xhtml/level2/html/HTMLTableElement19.xhtml
dom/html/level2/html/HTMLTableElement04.html
dom/html/level2/html/HTMLTableElement06.html
dom/html/level2/html/HTMLTableElement21.html
dom/xhtml/level2/html/HTMLTableElement22.xhtml
accessibility/table-detection.html
dom/html/level2/html/table03.html
dom/xhtml/level2/html/HTMLTableElement24.xhtml
dom/html/level2/html/HTMLTableElement19.html
dom/xhtml/level2/html/HTMLTableElement28.xhtml
dom/xhtml/level2/html/HTMLCollection08.xhtml
dom/xhtml/level2/html/HTMLTableRowElement01.xhtml
dom/xhtml/level2/html/HTMLTableElement30.xhtml
accessibility/th-as-title-ui.html
dom/xhtml/level2/html/HTMLCollection01.xhtml
dom/html/level2/html/HTMLCollection08.html
accessibility/table-with-rules.html
dom/xhtml/level2/html/HTMLCollection06.xhtml
dom/html/level2/html/HTMLCollection07.html
dom/html/level2/html/HTMLCollection01.html

[Build Bot]

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

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

[Ryosuke Niwa]

Comment on attachment 217406 [details]
Patch

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

> Source/WebCore/html/HTMLTableElement.cpp:76
> +    if (auto firstCaption = childrenOfType<HTMLTableCaptionElement>(*this).first())
> +        return const_cast<HTMLTableCaptionElement*>(firstCaption);
> +
> +    return nullptr;

Can't we just do:
return const_cast<HTMLTableCaptionElement*>(childrenOfType<HTMLTableCaptionElement>(*this).first());
?

[Build Bot]

Comment on attachment 217406 [details]
Patch

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

New failing tests:
dom/html/level2/html/HTMLCollection06.html
dom/xhtml/level2/html/HTMLTableElement21.xhtml
dom/html/level2/html/HTMLTableElement28.html
dom/html/level2/html/HTMLTableElement22.html
dom/html/level2/html/HTMLTableRowElement01.html
accessibility/table-one-cell.html
dom/html/level2/html/HTMLTableElement24.html
dom/html/level2/html/HTMLTableElement30.html
dom/xhtml/level2/html/HTMLCollection07.xhtml
dom/xhtml/level2/html/HTMLTableElement19.xhtml
dom/html/level2/html/HTMLTableElement04.html
dom/html/level2/html/HTMLTableElement06.html
dom/html/level2/html/HTMLTableElement21.html
dom/xhtml/level2/html/HTMLTableElement22.xhtml
accessibility/table-detection.html
dom/html/level2/html/table03.html
dom/xhtml/level2/html/HTMLTableElement24.xhtml
dom/html/level2/html/HTMLTableElement19.html
dom/xhtml/level2/html/HTMLTableElement28.xhtml
accessibility/table-nofirstbody.html
dom/xhtml/level2/html/HTMLCollection08.xhtml
dom/xhtml/level2/html/HTMLTableElement30.xhtml
accessibility/th-as-title-ui.html
dom/xhtml/level2/html/HTMLCollection01.xhtml
dom/html/level2/html/HTMLCollection08.html
accessibility/table-with-rules.html
dom/xhtml/level2/html/HTMLCollection06.xhtml
dom/html/level2/html/HTMLCollection07.html
dom/html/level2/html/HTMLCollection01.html
accessibility/non-data-table-cell-title-ui-element.html

[Build Bot]

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

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

[Darin Adler]

Comment on attachment 217406 [details]
Patch

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

>> Source/WebCore/html/HTMLTableElement.cpp:76
>> +    if (auto firstCaption = childrenOfType<HTMLTableCaptionElement>(*this).first())
>> +        return const_cast<HTMLTableCaptionElement*>(firstCaption);
>> +
>> +    return nullptr;
> 
> Can't we just do:
> return const_cast<HTMLTableCaptionElement*>(childrenOfType<HTMLTableCaptionElement>(*this).first());
> ?

Yes, right, there’s no need for the if statement. A null pointer can be casted.

> Source/WebCore/html/HTMLTableElement.cpp:90
> +    if (auto firstSection = childrenOfType<HTMLTableSectionElement>(*this).first())
> +        return const_cast<HTMLTableSectionElement*>(firstSection);
> +
> +    return nullptr;

This one is wrong. It needs to only return a thead, not any HTMLTableSectionElement. So there needs to be a loop. We could still use childrenOfType, but we need a loop.

> Source/WebCore/html/HTMLTableElement.cpp:110
> +    if (auto firstSection = childrenOfType<HTMLTableSectionElement>(*this).first())
> +        return const_cast<HTMLTableSectionElement*>(firstSection);
> +
> +    return nullptr;

This one is wrong. It needs to only return a tfoot, not any HTMLTableSectionElement. So there needs to be a loop. We could still use childrenOfType, but we need a loop.

> Source/WebCore/html/HTMLTableElement.cpp:180
> -    for (Node* child = lastChild(); child; child = child->previousSibling()) {
> -        if (child->hasTagName(tbodyTag))
> -            return toHTMLTableSectionElement(child);
> -    }
> -    return 0;
> +    if (auto lastSection = childrenOfType<HTMLTableSectionElement>(*this).last())
> +        return const_cast<HTMLTableSectionElement*>(lastSection);
> +
> +    return nullptr;

This one is wrong. It needs to only return a tbody, not any HTMLTableSectionElement. So there needs to be a loop. We could still use childrenOfType, but we need a loop.

[Gyuyoung Kim]

This patch can be fixed after solving an issue mentioned in
https://bugs.webkit.org/show_bug.cgi?id=124571#c14

[Anne van Kesteren]

Pull request: https://github.com/WebKit/WebKit/pull/22269

[EWS]

Committed 272797@main (7cc92197012b): <https://commits.webkit.org/272797@main>

Reviewed commits have been landed. Closing PR #22269 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/120685179>