Bug 137056 - Add initial is<>() / downcast<>() support for any type of Nodes
Summary: Add initial is<>() / downcast<>() support for any type of Nodes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 136839
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
  Show dependency treegraph
 
Reported: 2014-09-23 21:21 PDT by Chris Dumez
Modified: 2014-10-14 20:01 PDT (History)
8 users (show)

See Also:


Attachments
WIP Patch (29.03 KB, patch)
2014-09-23 21:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (35.22 KB, patch)
2014-09-23 21:37 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (51.67 KB, patch)
2014-09-24 19:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.