Get rid of unnecessary isChildNodeList() virtual call in clearChildNodeListCache()

Get rid of unnecessary isChildNodeList() virtual call in clearChildNodeListCache()

This CL inlines clearChildNodeListCache() in the caller as there is only one to
avoid an unnecessary isChildNodeList() virtual call. The calling method is
invalidateNodeListCachesInAncestors(), which this CL moves from Node to
ContainerNode to make sure it is only called for ContainerNodes (Only
ContainerNodes' node lists have caches that need invalidation).

R=adamk@chromium.org, esprehn@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179056

[adamk, 2014-07-24 22:06:20]

https://codereview.chromium.org/413763004/diff/1/Source/core/dom/NodeListsNod...
File Source/core/dom/NodeListsNodeData.h (right):

https://codereview.chromium.org/413763004/diff/1/Source/core/dom/NodeListsNod...
Source/core/dom/NodeListsNodeData.h:43: // This can only be called from
ContainerNode so casting to ChildNodeList
This seems a bit unsafe, since there's nothing (except maybe an ASSERT in
toChildNodeList) stopping this from getting called on a Node.

Is this a performance win?

[Inactive, 2014-07-25 00:09:50]

https://codereview.chromium.org/413763004/diff/1/Source/core/dom/NodeListsNod...
File Source/core/dom/NodeListsNodeData.h (right):

https://codereview.chromium.org/413763004/diff/1/Source/core/dom/NodeListsNod...
Source/core/dom/NodeListsNodeData.h:43: // This can only be called from
ContainerNode so casting to ChildNodeList
On 2014/07/24 22:06:20, adamk wrote:
> This seems a bit unsafe, since there's nothing (except maybe an ASSERT in
> toChildNodeList) stopping this from getting called on a Node.

Yes, there is an assertion in the casting function but I understand your
concern. Before I give up on this, I'd like to make a counter proposal:
- Remove the NodeListsNodeData::clearChildNodeListCache() method
- Add a NodeList* NodeListsNodeData::childNodeList() getter instead
- From ContainerNode::invalidateNodeListCachesInAncestors(), call
toChildNodeList(nodeLists->childNodeList())->invalidateCache()

Since we are in a ContainerNode method, the container node would look safer.
Loosing the clearChildNodeListCache() method does not seem important as there is
only 1 caller.

Maybe even better, the getter could even look like:
ChildNodeList* childNodeList(ContainerNode&);
// similarly to ensureChildNodeList(ContainerNode&) below.

What do you think?

> Is this a performance win?
I have just run Dromaeo but it did not show any perf win. I introduced this
virtual function call a while back and was not entirely happy with it, which is
why I was revisiting it.

[Inactive, 2014-07-25 12:41:05]

I updated the patch as per my counter proposal. I hope this addresses your
concerns. PTAL.

[adamk, 2014-07-28 18:35:31]

lgtm

Thanks, this approach is nice. The unused ContainerNode& argument is a little
weird but at least it's symmetric with the ensure() methods.

[Inactive, 2014-07-28 20:27:37]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-07-28 20:28:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/413763004/20001

[commit-bot: I haz the power, 2014-07-28 20:29:40]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[commit-bot: I haz the power, 2014-07-28 20:32:46]

Change committed as 179056