Bug 124571 - Generate toHTMLFooElement() to clean up static_cast<>
Summary: Generate toHTMLFooElement() to clean up static_cast<>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on: 124669
Blocks:
  Show dependency treegraph
 
Reported: 2013-11-19 02:38 PST by Gyuyoung Kim
Modified: 2013-11-20 20:43 PST (History)
5 users (show)

See Also:


Attachments
Patch (18.02 KB, patch)
2013-11-19 02:40 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (18.05 KB, patch)
2013-11-19 05:03 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (19.10 KB, patch)
2013-11-19 18:27 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (18.84 KB, patch)
2013-11-19 18:36 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (18.87 KB, patch)
2013-11-19 22:16 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (15.91 KB, patch)
2013-11-20 18:01 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (15.92 KB, patch)
2013-11-20 19:33 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2013-11-19 02:38:00 PST
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().
Comment 1 Gyuyoung Kim 2013-11-19 02:40:18 PST
Created attachment 217282 [details]
Patch
Comment 2 EFL EWS Bot 2013-11-19 04:21:55 PST
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
Comment 3 Gyuyoung Kim 2013-11-19 05:03:36 PST
Created attachment 217288 [details]
Patch
Comment 4 Darin Adler 2013-11-19 09:35:01 PST
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
Comment 5 Gyuyoung Kim 2013-11-19 18:27:55 PST
Created attachment 217367 [details]
Patch
Comment 6 Gyuyoung Kim 2013-11-19 18:30:28 PST
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());
Comment 7 Gyuyoung Kim 2013-11-19 18:36:34 PST
Created attachment 217371 [details]
Patch
Comment 8 Gyuyoung Kim 2013-11-19 21:17:09 PST
(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.
Comment 9 Gyuyoung Kim 2013-11-19 22:16:16 PST
Created attachment 217382 [details]
Patch
Comment 10 WebKit Commit Bot 2013-11-19 23:19:16 PST
Comment on attachment 217382 [details]
Patch

Clearing flags on attachment: 217382

Committed r159551: <http://trac.webkit.org/changeset/159551>
Comment 11 WebKit Commit Bot 2013-11-19 23:19:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Commit Bot 2013-11-20 10:14:49 PST
Re-opened since this is blocked by bug 124669
Comment 13 Antti Koivisto 2013-11-20 10:18:25 PST
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.
Comment 14 Darin Adler 2013-11-20 11:00:41 PST
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.
Comment 15 Gyuyoung Kim 2013-11-20 18:01:09 PST
Created attachment 217509 [details]
Patch
Comment 16 Gyuyoung Kim 2013-11-20 18:03:10 PST
(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.
Comment 17 Gyuyoung Kim 2013-11-20 19:31:01 PST
Comment on attachment 217509 [details]
Patch

There is no regression on Mac with this patch.
Comment 18 Gyuyoung Kim 2013-11-20 19:33:48 PST
Created attachment 217513 [details]
Patch
Comment 19 WebKit Commit Bot 2013-11-20 20:43:25 PST
Comment on attachment 217513 [details]
Patch

Clearing flags on attachment: 217513

Committed r159604: <http://trac.webkit.org/changeset/159604>
Comment 20 WebKit Commit Bot 2013-11-20 20:43:29 PST
All reviewed patches have been landed.  Closing bug.