[webkit-reviews] review denied: [Bug 124645] Use childrenOfType<> instead of using a loop in HTMLTableElement : [Attachment 217406] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 124645: Use childrenOfType<> instead of using a loop in HTMLTableElement
https://bugs.webkit.org/show_bug.cgi?id=124645

Attachment 217406: Patch
https://bugs.webkit.org/attachment.cgi?id=217406&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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>(*t
his).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.