Bug 21158 – reduce use of virtual functions in Node for speed

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Darin Adler]

Use the element and container bits more.

[Darin Adler]

Created attachment 23864 [details]
patch

[Sam Weinig]

Comment on attachment 23864 [details]
patch

This looks good, but I am concerned about the change to ChildNodeList.  A ChildNodeList can contain non-Elements members and you have changed nodeMatches to take an Element (which makes the name weird now).  Does this work because nodeMatches is never called for a ChildNodeList as it implements its own item method?  If that is the case, can we remove the nodeMatches method from this subclass?

[Darin Adler]

(In reply to comment #2)
> Does this work because
> nodeMatches is never called for a ChildNodeList as it implements its own item
> method?

Yes, that's why. The nodeMatches function there was just dead code!

There was no caller of nodeMatches that ever called it without first checking it was an Element. So the patch makes no behavior change.

I agree that we could give nodeMatches a better name. I didn't think of one while making this change; elementMatches does not seem like a better name. I also figured renaming in a separate patch would be better.

> If that is the case, can we remove the nodeMatches method from this
> subclass?

We can't, because nodeMatches is a pure virtual function in DynamicNodeList. But we could have it be empty with an ASSERT_NOT_REACHED() in it.

A cleaner fix would be to make all a class derived from DynamicNodeList that implements, length, item, and itemWithName and moving nodeMatches to that derived class, then using that as the base class for TagNodeList, NameNodeList, and ClassNodeList.

I really don't feel strongly about either of these.

[Darin Adler]

Hmm, no, that's wrong. It looks like nodeMatches can get called by itemWithName. But since a non-Element can't have a name, that's still fine.

I think the patch is probably fine as-is. I didn't change the design at all. Let me know what you think.

[Sam Weinig]

Comment on attachment 23864 [details]
patch

Ah, I forgot about the itemWithName case.  That makes sense.  r=me

[Darin Adler]

http://trac.webkit.org/changeset/37037

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.