Bug 137056

Summary: Add initial is<>() / downcast<>() support for any type of Nodes
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, commit-queue, darin, dbates, kling, koivisto, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 136839    
Bug Blocks: 137106, 137128, 137137, 137169, 137172, 137183, 137184, 137188, 137199, 137221, 137222, 137241, 137243, 137270, 137284, 137286, 137318, 137331, 137364, 137381, 137398, 137424, 137429, 137430, 137431, 137432, 137436, 137437, 137438, 137439, 137440, 137449, 137451, 137477, 137497, 137549, 137591, 137595, 137625, 137644, 137661, 137722, 137731    
Attachments:
Description Flags
WIP Patch
none
Patch
none
Patch none

Description Chris Dumez 2014-09-23 21:21:44 PDT
Add initial is<>() / downcast<>() support for any type of Nodes, not just Elements by:
- Moving the is<>() / downcast<>() declaration from Element.h to Node.h
- Introducing a DEFINE_TYPE_HELPERS_*() macro that generates the needed template specializations for is<>() / downcast<>() to work. This macro will replace NODE_TYPE_CASTS() entirely once the code base is fully ported.
Comment 1 Chris Dumez 2014-09-23 21:29:26 PDT
Created attachment 238584 [details]
WIP Patch
Comment 2 WebKit Commit Bot 2014-09-23 21:31:42 PDT
Attachment 238584 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLFormControlElement.h:195:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Dumez 2014-09-23 21:37:33 PDT
Created attachment 238585 [details]
Patch
Comment 4 WebKit Commit Bot 2014-09-23 21:38:47 PDT
Attachment 238585 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLFormControlElement.h:195:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Chris Dumez 2014-09-23 22:57:06 PDT
Comment on attachment 238585 [details]
Patch

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

> Source/WebCore/dom/DocumentFragment.h:55
> +DEFINE_TYPE_HELPERS_BEGIN(DocumentFragment)

The reason the macro has a BEGIN / END is so that isDocumentFragment() can be private. We don't want to have both isDocumentFragment() and is<DocumentFragment>().

> Source/WebCore/dom/Node.h:742
> +template <typename ExpectedType, typename ArgType>

This is all moved from Element.h

> Source/WebCore/html/HTMLMediaElement.h:918
> +struct NodeTypeCastTraits<const HTMLMediaElement, ArgType> {

These explicit specializations will go away once I use DEFINE_TYPE_HELPERS_*() everywhere. I only ported 2 classes in this patch to minimize the size of the change.
Comment 6 Benjamin Poulain 2014-09-24 15:31:39 PDT
Comment on attachment 238585 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:-742
>      if (isHTMLTextAreaElement(node))
> -        return toHTMLFormControlElement(node)->isReadOnly();

Uh, that's interesting...

>> Source/WebCore/dom/DocumentFragment.h:55
>> +DEFINE_TYPE_HELPERS_BEGIN(DocumentFragment)
> 
> The reason the macro has a BEGIN / END is so that isDocumentFragment() can be private. We don't want to have both isDocumentFragment() and is<DocumentFragment>().

Interesting take to simplify the definition.

I am not sure with the name. "Helpers" is never really helpful for a name.

Why not simply SPECIALIZE_TYPE_TRAITS or something like that?

> Source/WebCore/dom/Node.h:784
> +// Add support for type checking / casting using is<>() / downcast<>() helpers
> +// for a specific class.

One line.

> Source/WebCore/html/HTMLFormControlElement.cpp:511
>          if (node->isElementNode() && toElement(node)->isFormControlElement())

Why not is<HTMLFormControlElement>(node) ?

> Source/WebCore/html/HTMLFormControlElement.h:195
> +    static bool isHTMLFormControlElement(const Node& node) { return node.isElementNode() && toElement(node).isFormControlElement(); }

return node.isElementNode() && isHTMLFormControlElement(toElement(node));

> Source/WebCore/html/HTMLFormElement.cpp:212
>      return 0;

nullptr

> Source/WebCore/html/HTMLFormElement.cpp:706
>      return 0;

nullptr
Comment 7 Chris Dumez 2014-09-24 19:40:02 PDT
Created attachment 238636 [details]
Patch
Comment 8 WebKit Commit Bot 2014-09-24 19:42:12 PDT
Attachment 238636 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLFormControlElement.h:195:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 WebKit Commit Bot 2014-09-24 20:50:46 PDT
Comment on attachment 238636 [details]
Patch

Clearing flags on attachment: 238636

Committed r173944: <http://trac.webkit.org/changeset/173944>
Comment 10 WebKit Commit Bot 2014-09-24 20:50:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Daniel Bates 2014-09-25 08:29:28 PDT
(In reply to comment #9)
> (From update of attachment 238636 [details])
> Clearing flags on attachment: 238636
> 
> Committed r173944: <http://trac.webkit.org/changeset/173944>

This broke the iOS build. See <rdar://problem/18454708> for more details.
Comment 12 Chris Dumez 2014-09-25 10:26:47 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 238636 [details] [details])
> > Clearing flags on attachment: 238636
> > 
> > Committed r173944: <http://trac.webkit.org/changeset/173944>
> 
> This broke the iOS build. See <rdar://problem/18454708> for more details.

This should be fixed in http://trac.webkit.org/changeset/173971. Thanks for notifying me.