[webkit-reviews] review denied: [Bug 103372] checkAcceptChid() needs fewer virtual calls : [Attachment 176225] Updated to ToT

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Hajime Morrita <morrita at google.com>'s
request for review:
Bug 103372: checkAcceptChid() needs fewer virtual calls
https://bugs.webkit.org/show_bug.cgi?id=103372

Attachment 176225: Updated to ToT
https://bugs.webkit.org/attachment.cgi?id=176225&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=176225&action=review


I like the overall direction of this patch, but it needs a bit more work.

> Source/WebCore/ChangeLog:24
> +	   (WebCore::isChildTypeAllowed): Adopted isDocumentTypeNode(), which
is faster than nodeType().

Why is this faster? isDocumentTypeNode just calls nodeType(), no?

> Source/WebCore/dom/ContainerNode.cpp:135
> +static inline bool isChildTypeAllowed(Node* newParent, Node* child)

Can we make these non-static, private methods? They all take "this" as the
first argument in all the callers.

> Source/WebCore/dom/ContainerNode.cpp:147
> +static inline void checkAcceptChild(Node* newParent, Node* newChild, Node*
oldChild, ExceptionCode& ec)

At this point, may as well rename this to checkAddChild.

> Source/WebCore/dom/ContainerNode.cpp:173
> +	   if (oldChild && newParent->isDocumentNode() &&
!toDocument(newParent)->canReplaceChild(newChild, oldChild)) {
> +	       ec = HIERARCHY_REQUEST_ERR;
> +	       return;
> +	   }

Can you just keep this in checkReplaceChild? You can do this call before
calling checkAcceptChild. It seems unlikely to me that you gain much
performance benefit by putting it here and it makes the code more confusing
since now checkAcceptChild also doubles as checking whether the child can be
replaced.

If you find that this does measurably improve performance, then maybe we should
consider it (maybe even as a separate followup patch?), but for now, I don't
think it's worth the confusion.

> Source/WebCore/dom/ContainerNode.cpp:178
> +	   if (!isChildTypeAllowed(newParent, newChild)) {
> +	       ec = HIERARCHY_REQUEST_ERR;
> +	       return;
> +	   }

It looks to me that moving this here from checkAddChild fixes a bug with
replaceChild. Mind writing a test for this case? Also, you could commit that
change first, which would simplify this patch since then you'd just be moving
the function.

> Source/WebCore/dom/ContainerNode.cpp:182
> +    // HIERARCHY_REQUEST_ERR: Raised if this node is of a type that does not
allow children of the type of the
> +    // newChild node, or if the node to append is one of this node's
ancestors.

I know this is just copy-pasted, but I don't think this comment adds any value.
It's obvious what the code below is doing.

> Source/WebCore/dom/ContainerNode.cpp:186
> +    if (newChild->contains(newParent)) {
> +	   ec = HIERARCHY_REQUEST_ERR;
> +	   return;

If you move this above the previous if-else statement, you could early return
in the if and then not need the else statement. Would make this code a bit
easier to read. I know that this is the slow part of this function, but I think
it's fine if we're a little slower in error cases. Error cases should almost
never be hit and are unlikely to be a performance bottleneck with these APIs I
think.

> Source/WebCore/dom/ContainerNode.cpp:199
> +    if (ec)
> +	   return;

Don't need this.

> Source/WebCore/dom/ContainerNode.cpp:203
> +// This method is used to do strict error-checking when adding children via
> +// the public DOM API (e.g., appendChild()).

I know this comment was in the previous version of the code, but I don't think
it actually adds any value. All our methods that take ExceptionCodes are for
servicing public DOM apis.

> Source/WebCore/dom/ContainerNode.cpp:208
> +    if (ec)
> +	   return;

Don't need this.