[webkit-reviews] review granted: [Bug 136789] Rename Node::childNodeCount() to countChildNodes() and avoid inefficient uses : [Attachment 238077] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 12 17:47:50 PDT 2014


Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 136789: Rename Node::childNodeCount() to  countChildNodes() and avoid
inefficient uses
https://bugs.webkit.org/show_bug.cgi?id=136789

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

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


If you are renaming childNodeCount for this reason, maybe you should rename
nodeIndex for the same reason.

Amazing how many call sites should have been using hasChildNodes!

> Source/WebCore/dom/ContainerNode.cpp:938
> -    Node *n;
> -    for (n = firstChild(); n; n = n->nextSibling())
> -	   count++;
> +    for (Node* n = firstChild(); n; n = n->nextSibling())
> +	   ++count;

Since you are touching this, why not change the variable name from "n" to
"child".

> Source/WebCore/editing/markup.cpp:749
> +    HTMLDivElement& div = toHTMLDivElement(*node);

I think I would call this “element” rather than “div”.

> Source/WebCore/editing/markup.cpp:757
> +    // FIXME: It is inefficient to call countChildNodes() just to check if
there are exactly 2 children.
> +    return div.countChildNodes() == 2 &&
isTabSpanTextNode(div.firstChild()->firstChild()) &&
div.firstChild()->nextSibling()->isTextNode();

Why not rewrite this instead of adding the FIXME? One way is to add a
hasTwoChildNodes helper function (doesn’t have to be a member of Node). Another
is to write it like this:

    Node* firstChild = div.firstChild();
    if (!firstChild)
	return;

    Node* secondChild = firstChild->nextSibling();

    if (!secondChild)
	return firstChild->isTextNode() || firstChild->firstChild();

    if (secondChild->nextSibling())
	return false;

    return isTabSpanTextNode(firstChild->firstChild()) &&
secondChild->isTextNode();

> Source/WebCore/html/shadow/MediaControlElements.cpp:-300
> -	   Node* child = childNode(i);

Wow, can’t believe anyone was doing this!

> Source/WebCore/html/shadow/MediaControlElements.cpp:300
> +    for (auto& element : childrenOfType<Element>(*this)) {

Can be childrenOfType<MediaControlTimeDisplayElement>?

> Source/WebCore/html/shadow/MediaControlElements.cpp:1153
> +    if (countChildNodes() < activeCues.size())
>	   removeChildren();

Even this might need a FIXME. Checking if we have less than a certain number of
child nodes doesn’t need to iterate them all. But maybe in practice the number
of nodes here is bounded at a small number.

> Source/WebCore/html/track/VTTRegion.cpp:377
> +	   if (m_scrollTimer.isActive())
> +	       break;

I know this is not new code, but this is a strange way to structure the loop.
Seems like we should just break after the call the startTimer, unless it
sometimes doesn’t start a timer.

> Source/WebCore/html/track/VTTRegion.cpp:379
> +	   float childTop = child.getBoundingClientRect()->top();
> +	   float childBottom = child.getBoundingClientRect()->bottom();

Quite inefficient to call getBoundingClientRect twice.


More information about the webkit-reviews mailing list