Bug 124645 - Use childrenOfType<> more in HTMLTableElement
Summary: Use childrenOfType<> more in HTMLTableElement
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Anne van Kesteren
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-20 01:23 PST by Gyuyoung Kim
Modified: 2024-01-08 19:07 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.95 KB, patch)
2013-11-20 01:25 PST, Gyuyoung Kim
darin: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (235.20 KB, application/zip)
2013-11-20 04:59 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (215.57 KB, application/zip)
2013-11-20 07:09 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2013-11-20 01:23:51 PST
No need to use a loop to get first child in a tree. childrenOfType<> provides a functionality to find it easily.
Comment 1 Gyuyoung Kim 2013-11-20 01:25:04 PST
Created attachment 217406 [details]
Patch
Comment 2 Gyuyoung Kim 2013-11-20 01:33:18 PST
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 ?
Comment 3 Build Bot 2013-11-20 04:59:12 PST
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
Comment 4 Build Bot 2013-11-20 04:59:16 PST
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
Comment 5 Ryosuke Niwa 2013-11-20 05:47:48 PST
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());
?
Comment 6 Build Bot 2013-11-20 07:09:01 PST
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
Comment 7 Build Bot 2013-11-20 07:09:05 PST
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
Comment 8 Darin Adler 2013-11-20 09:34:36 PST
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.
Comment 9 Gyuyoung Kim 2013-11-20 20:32:43 PST
This patch can be fixed after solving an issue mentioned in
https://bugs.webkit.org/show_bug.cgi?id=124571#c14
Comment 10 Anne van Kesteren 2023-12-30 01:51:49 PST
Pull request: https://github.com/WebKit/WebKit/pull/22269
Comment 11 EWS 2024-01-08 19:06:27 PST
Committed 272797@main (7cc92197012b): <https://commits.webkit.org/272797@main>

Reviewed commits have been landed. Closing PR #22269 and removing active labels.
Comment 12 Radar WebKit Bug Importer 2024-01-08 19:07:15 PST
<rdar://problem/120685179>