Bug 124571 – Generate toHTMLFooElement() to clean up static_cast<>

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

Though there are a lot of clean up commits before, there are still use of static_cast<HTMLFooElement*>. To clean up them, we need to generate toHTMLDetails|Meta|Summary|TableSection|TableCaptionElement().

[Gyuyoung Kim]

Created attachment 217282 [details]
Patch

[EFL EWS Bot]

Comment on attachment 217282 [details]
Patch

Attachment 217282 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/28828050

[Gyuyoung Kim]

Created attachment 217288 [details]
Patch

[Darin Adler]

Comment on attachment 217288 [details]
Patch

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

Patch is fine as is. I see some obvious opportunities for cleanup that could be done as part of this or separately.

> Source/WebCore/html/HTMLTableElement.cpp:76
>      for (Node* child = firstChild(); child; child = child->nextSibling()) {
>          if (child->hasTagName(captionTag))
> -            return static_cast<HTMLTableCaptionElement*>(child);
> +            return toHTMLTableCaptionElement(child);
>      }
>      return 0;

This entire function should just be:

    return childrenOfType<HTMLTableCaptionElement>(*this).first();

No need for a loop at all.

> Source/WebCore/html/HTMLTableRowElement.cpp:84
> +            HTMLTableSectionElement* section = toHTMLTableSectionElement(node);

Should use a reference rather than a pointer here for this local.

> Source/WebCore/html/MediaDocument.cpp:96
> +    HTMLSourceElement* source = toHTMLSourceElement(sourceElement.get());

Should use a reference rather than a pointer here for this local.

> Source/WebCore/html/shadow/DetailsMarkerControl.cpp:67
>      Element* element = shadowHost();
> -    ASSERT_WITH_SECURITY_IMPLICATION(!element || element->hasTagName(summaryTag));
> -    return static_cast<HTMLSummaryElement*>(element);
> +    return toHTMLSummaryElement(element);

Doesn’t seem like we need the local variable here any more.

> Source/WebCore/page/SpatialNavigation.cpp:761
> +    return candidate.isFrameOwnerElement() ? toHTMLFrameOwnerElement(candidate.visibleNode) : 0;

nullptr

[Gyuyoung Kim]

Created attachment 217367 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 217288 [details]
Patch

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

Fixed all other nits.

>> Source/WebCore/html/HTMLTableElement.cpp:76
>>      return 0;
> 
> This entire function should just be:
> 
>     return childrenOfType<HTMLTableCaptionElement>(*this).first();
> 
> No need for a loop at all.

I would like to fix this on new bug. I'm going to file a bug, then adding you to CC.

>> Source/WebCore/html/MediaDocument.cpp:96
>> +    HTMLSourceElement* source = toHTMLSourceElement(sourceElement.get());
> 
> Should use a reference rather than a pointer here for this local.

Darin, I'd like to check if you mean below style before landing.

HTMLSourceElement& source = *toHTMLSourceElement(sourceElement.get());

[Gyuyoung Kim]

Created attachment 217371 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #6)
> 
> HTMLSourceElement& source = *toHTMLSourceElement(sourceElement.get());

Darin, if this isn't correct, please let me know. I believe this patch is fine as well.

[Gyuyoung Kim]

Created attachment 217382 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 217382 [details]
Patch

Clearing flags on attachment: 217382

Committed r159551: <http://trac.webkit.org/changeset/159551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 124669

[Antti Koivisto]

This hits asserts

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r159551%20(576)/results.html

ASSERTION FAILED: !node || (WebCore::isHTMLTableSectionElement(*node))
/Volumes/Data/slave/mavericks-debug/build/Source/WebCore/html/HTMLTableSectionElement.h(62) : WebCore::HTMLTableSectionElement *WebCore::toHTMLTableSectionElement(WebCore::Node *)
1   0x10b8f4f70 WTFCrash
2   0x10d118b0f WebCore::toHTMLTableSectionElement(WebCore::Node*)
3   0x10d115d5d WebCore::HTMLTableElement::tFoot() const
4   0x10d5aefcf WebCore::jsHTMLTableElementTFoot(JSC::ExecState*, JSC::JSValue, JSC::PropertyName)
5   0x10b2b554f JSC::PropertySlot::getValue(JSC::ExecState*, JSC::PropertyName) const
6   0x10b2d0153 JSC::JSValue::get(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&) const
7   0x10b715ccb llint_slow_path_get_by_id
8   0x10b7209af llint_op_get_by_id

Some of the assertions are probably wrong. Note that multiple tags map to HTMLTableSectionElement.

[Darin Adler]

Comment on attachment 217382 [details]
Patch

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

> Source/WebCore/html/HTMLTagNames.in:121
> +tbody interfaceName=HTMLTableSectionElement, generateTypeHelpers

Aha. This is wrong, and I should have spotted that during my original review! tbody is only one of the three tags that maps to HTMLTableSectionElement

You should re-land this patch with the parts other than the HTMLTableSectionElement part, and then tackle that separately.

[Gyuyoung Kim]

Created attachment 217509 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #14)
> (From update of attachment 217382 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217382&action=review
> 
> > Source/WebCore/html/HTMLTagNames.in:121
> > +tbody interfaceName=HTMLTableSectionElement, generateTypeHelpers
> 
> Aha. This is wrong, and I should have spotted that during my original review! tbody is only one of the three tags that maps to HTMLTableSectionElement
> 
> You should re-land this patch with the parts other than the HTMLTableSectionElement part, and then tackle that separately.

Darin, I'm sorry. It was my fault. I remove to support toHTMLTableSectionElement() in this patch. I will take a look it on new bug. I will request a review after checking on Mac. Thanks.

[Gyuyoung Kim]

Comment on attachment 217509 [details]
Patch

There is no regression on Mac with this patch.

[Gyuyoung Kim]

Created attachment 217513 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 217513 [details]
Patch

Clearing flags on attachment: 217513

Committed r159604: <http://trac.webkit.org/changeset/159604>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.