[webkit-reviews] review granted: [Bug 136719] Avoid redundant isElementNode() checks in Traversal<HTML*Element> / Traversal<SVG*Element> : [Attachment 237977] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 136719: Avoid redundant isElementNode() checks in Traversal<HTML*Element> /
Traversal<SVG*Element>
https://bugs.webkit.org/show_bug.cgi?id=136719

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237977&action=review


I think we don’t need multiple function templates when the isXXX function is
already overloaded for all the types we care about. It would be nice to keep
the explicit mention of Node to a minimum.

> Source/WebCore/ChangeLog:41
> +	   Add new isElementOfType() helper taking a Node in argument.

I don’t understand why we need this. It seems like a step in the wrong
direction. One of the main reasons we have these helpers is to create a future
where we code in terms of Element and not Node.

Is there a way to accomplish everything else without adding all these Node
overloads in all the files?

I understand that somehow we want to express generically that checking
isHTMLElement also implicitly checks isElement and avoid a second check. Is
there some other way to do this?

> Source/WebCore/html/HTMLFormControlElement.h:197
>  inline bool isHTMLFormControlElement(const Element& element) { return
element.isFormControlElement(); }
>  inline bool isHTMLFormControlElement(const Node& node) { return
node.isElementNode() && toElement(node).isFormControlElement(); }
> +template <> inline bool isElementOfType<const HTMLFormControlElement>(const
Node& node) { return isHTMLFormControlElement(node); }
>  template <> inline bool isElementOfType<const HTMLFormControlElement>(const
Element& element) { return isHTMLFormControlElement(element); }

Node should come after Element to match the code above, which goes from more
specific to more general.

Can this be a single template rather than two functions? The argument type
would need to be a template argument, but we wouldn’t have to specify Element
and Node.

> Source/WebCore/html/HTMLFrameElementBase.h:81
> +inline bool isHTMLFrameElementBase(const HTMLElement& element) { return
isHTMLFrameElement(element) || isHTMLIFrameElement(element); }
> +inline bool isHTMLFrameElementBase(const Node& node) { return
isHTMLFrameElement(node) || isHTMLIFrameElement(node); }

Can this be a single template rather than two functions? The argument type
would need to be a template argument, but we wouldn’t have to specify
HTMLElement and Node.

> Source/WebCore/html/HTMLMediaElement.h:920
>  void isHTMLMediaElement(const HTMLMediaElement&); // Catch unnecessary
runtime check of type known at compile time.
>  inline bool isHTMLMediaElement(const Element& element) { return
element.isMediaElement(); }
>  inline bool isHTMLMediaElement(const Node& node) { return
node.isElementNode() && toElement(node).isMediaElement(); }
> -template <> inline bool isElementOfType<const HTMLMediaElement>(const
Element& element) { return element.isMediaElement(); }
> +template <> inline bool isElementOfType<const HTMLMediaElement>(const Node&
node) { return isHTMLMediaElement(node); }
> +template <> inline bool isElementOfType<const HTMLMediaElement>(const
Element& element) { return isHTMLMediaElement(element); }

Node should come after Element to match the code above, which goes from more
specific to more general.

But can this be a single template rather than two functions? The argument type
would need to be a template argument, but we wouldn’t have to specify Element
and Node.

> Source/WebCore/html/LabelableElement.h:57
> -template <> inline bool isElementOfType<const LabelableElement>(const
Element& element) { return isLabelableElement(element); }
> +template <> inline bool isElementOfType<const LabelableElement>(const
HTMLElement& element) { return isLabelableElement(element); }
> +template <> inline bool isElementOfType<const LabelableElement>(const Node&
node) { return isLabelableElement(node); }

Can this be a single template rather than two functions? The argument type
would need to be a template argument, but we wouldn’t have to specify
HTMLElement and Node.

> Source/WebCore/mathml/MathMLElement.h:82
>  inline bool isMathMLElement(const Node& node) { return
node.isMathMLElement(); }
>  NODE_TYPE_CASTS(MathMLElement)
>  
> +template <typename T> bool isElementOfType(const MathMLElement&);
> +template <> inline bool isElementOfType<const MathMLElement>(const Node&
node) { return node.isMathMLElement(); }
> +template <> inline bool isElementOfType<const MathMLElement>(const
MathMLElement&) { return true; }

Node should come after MathMLElement to match the code above, which goes from
specific to general. This should also come before the NODE_TYPE_CASTS macro to
match the other files.

> Source/WebCore/svg/SVGElement.h:230
> -template <> inline bool isElementOfType<const SVGElement>(const Element&
element) { return element.isSVGElement(); }
> +template <typename T> bool isElementOfType(const SVGElement&);
> +template <> inline bool isElementOfType<const SVGElement>(const Node& node)
{ return node.isSVGElement(); }
> +template <> inline bool isElementOfType<const SVGElement>(const SVGElement&)
{ return true; }

Node should come after SVGElement to match the code above, which goes from more
specific to more general.

> Source/WebCore/svg/SVGFilterPrimitiveStandardAttributes.h:89
> +template <> inline bool isElementOfType<const
SVGFilterPrimitiveStandardAttributes>(const SVGElement& element) { return
isSVGFilterPrimitiveStandardAttributes(element); }
> +template <> inline bool isElementOfType<const
SVGFilterPrimitiveStandardAttributes>(const Node& node) { return
isSVGFilterPrimitiveStandardAttributes(node); }

Can this be a single template rather than two functions? The argument type
would need to be a template argument, but we wouldn’t have to specify
SVGElement and Node.

> Source/WebCore/svg/animation/SVGSMILElement.h:244
> +template <> inline bool isElementOfType<const SVGSMILElement>(const
SVGElement& element) { return isSVGSMILElement(element); }
> +template <> inline bool isElementOfType<const SVGSMILElement>(const Node&
node) { return isSVGSMILElement(node); }

Can this be a single template rather than two specializations? The argument
type would need to be a template argument, but we wouldn’t have to specify
HTMLElement and Node.