[webkit-reviews] review granted: [Bug 137056] Add initial is<>() / downcast<>() support for any type of Nodes : [Attachment 238585] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137056: Add initial is<>() / downcast<>() support for any type of Nodes
https://bugs.webkit.org/show_bug.cgi?id=137056

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

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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