[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