Bug 136789 – Rename Node::childNodeCount() to countChildNodes() and avoid inefficient uses

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Rename Node::childNodeCount() to  countChildNodes() to make it obvious it is doing computation and not merely returning a cached value.
Node::childNodeCount() is used in a lot of places where more efficient alternatives exist so we should fix those instances as well.

[Chris Dumez]

Created attachment 238054 [details]
WIP Patch

[Chris Dumez]

Created attachment 238075 [details]
Patch

[Chris Dumez]

Created attachment 238077 [details]
Patch

[Darin Adler]

Comment on attachment 238077 [details]
Patch

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.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 238077 [details])
> 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.

Good point. I'll take a look at this in a follow-up patch. Note that I an also planning to rename Node::childNode(index) to Node::traverseToChildAt(index) for the same reason.

[Chris Dumez]

Comment on attachment 238077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238077&action=review

>> Source/WebCore/editing/markup.cpp:757
>> +    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();

I did that because I was planning to add another helper on Node and the patch was already big. However, since we seem to only do this in one place, I don't think adding the helper is worth it and your proposal makes more sense. I'll update accordingly.

>> Source/WebCore/html/track/VTTRegion.cpp:379
>> +        float childBottom = child.getBoundingClientRect()->bottom();
> 
> Quite inefficient to call getBoundingClientRect twice.

True, I'll cache it in a variable.

[Chris Dumez]

Created attachment 238088 [details]
Patch

[Chris Dumez]

Comment on attachment 238077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238077&action=review

>> Source/WebCore/dom/ContainerNode.cpp:938
>> +        ++count;
> 
> Since you are touching this, why not change the variable name from "n" to "child".

Done.

>> Source/WebCore/editing/markup.cpp:749
>> +    HTMLDivElement& div = toHTMLDivElement(*node);
> 
> I think I would call this “element” rather than “div”.

Done.

>>> Source/WebCore/editing/markup.cpp:757
>>> +    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();
> 
> I did that because I was planning to add another helper on Node and the patch was already big. However, since we seem to only do this in one place, I don't think adding the helper is worth it and your proposal makes more sense. I'll update accordingly.

Done.

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

This actually doesn't work because there is no isElementOfType() template specialization for MediaControlTimeDisplayElement. It also does not seem there is a method to identify a MediaControlTimeDisplayElement at the moment.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1153
>>          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.

Done, I added the FIXME.

>> Source/WebCore/html/track/VTTRegion.cpp:377
>> +            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.

Done. It seems the timer is always started in startTimer(), unless it was already started.

>>> Source/WebCore/html/track/VTTRegion.cpp:379
>>> +        float childBottom = child.getBoundingClientRect()->bottom();
>> 
>> Quite inefficient to call getBoundingClientRect twice.
> 
> True, I'll cache it in a variable.

Done.

[WebKit Commit Bot]

Comment on attachment 238088 [details]
Patch

Clearing flags on attachment: 238088

Committed r173606: <http://trac.webkit.org/changeset/173606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 238088 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238088&action=review

> Source/WebCore/editing/htmlediting.cpp:348
>      // NOTE: This should preempt the childNodeCount for, e.g., select nodes

Dan, you should have waited to hear from at least one editing expert before rushing to check in a “what does this mean?” comment.

The concept here is that editing allows an offset of 1 to mean “after this element” for elements that act as an atomic unit for editing. That’s a bad design that we should get rid of some day, but this function is implementing that design. The comment means that for an item that is atomic for editing that happens to have child nodes, we should return "1", not the number of child nodes. A <select> element is such an element, treated as a single unit for editing, but has children. And the writer of that comment thinks that returning 1 for lastOffsetForEditing for that type of element will help other editing code work properly.

The better long term solution is to eliminate the magic offset of "1" to mean after such elements. That project is still worth doing, but probably requires quite a bit of work.

[Daniel Bates]

(In reply to comment #9)
> (From update of attachment 238088 [details])
> Clearing flags on attachment: 238088
> 
> Committed r173606: <http://trac.webkit.org/changeset/173606>

Committed iOS build fix in <http://trac.webkit.org/changeset/173608>.

[Daniel Bates]

(In reply to comment #11)
> (From update of attachment 238088 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238088&action=review
> 
> > Source/WebCore/editing/htmlediting.cpp:348
> >      // NOTE: This should preempt the childNodeCount for, e.g., select nodes
> 
> Dan, you should have waited to hear from at least one editing expert before rushing to check in a “what does this mean?” comment.
> 

I'll keep this in mind moving forward.

> The concept here is that editing allows an offset of 1 to mean “after this element” for elements that act as an atomic unit for editing. That’s a bad design that we should get rid of some day, but this function is implementing that design. The comment means that for an item that is atomic for editing that happens to have child nodes, we should return "1", not the number of child nodes. A <select> element is such an element, treated as a single unit for editing, but has children. 

Notice that we call editingIgnoresContent() (http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=173608#L349) only if Node::hasChildNodes() (http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=173608#L345) evaluates to false. That is, we hard coded "return 1" for a node n that does not have children and editingIgnoresContent(n) evaluates to true.

[Darin Adler]

(In reply to comment #13)
> Notice that we call editingIgnoresContent() (http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=173608#L349) only if Node::hasChildNodes() (http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=173608#L345) evaluates to false. That is, we hard coded "return 1" for a node n that does not have children and editingIgnoresContent(n) evaluates to true.

Yes. That behavior, and the possibility that it’s incorrect, is what the comment is calling attention to. The dependency on not having children doesn’t seem right, since editingIgnoresContent means we should ignore children.

[Ryosuke Niwa]

(In reply to comment #14)
> (In reply to comment #13)
> > Notice that we call editingIgnoresContent() (http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=173608#L349) only if Node::hasChildNodes() (http://trac.webkit.org/browser/trunk/Source/WebCore/editing/htmlediting.cpp?rev=173608#L345) evaluates to false. That is, we hard coded "return 1" for a node n that does not have children and editingIgnoresContent(n) evaluates to true.
> 
> Yes. That behavior, and the possibility that it’s incorrect, is what the comment is calling attention to. The dependency on not having children doesn’t seem right, since editingIgnoresContent means we should ignore children.

We use the magic number 1 to indicate the position is after the element in cases where we don't have children.  See the definition of Position::anchorTypeForLegacyEditingPosition.  It checks if the offset is 0, in which case, it assumes the position is before the anchor node.  Otherwise, it assumes the position is after the anchor node.  In lastOffsetForEditing, we're being conservative by always returning the greater of 1 or the number of children so that even the code that doesn't know anything about this magic number will still treat it as the position at the every end of the element.