Bug 21158

Summary: reduce use of virtual functions in Node for speed
Product: WebKit Reporter: Darin Adler <darin>
Component: DOMAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: OS X 10.5   
Attachments:
Description Flags
patch sam: review+

Darin Adler
Reported 2008-09-26 17:44:53 PDT
Use the element and container bits more.
Attachments
patch (29.84 KB, patch)
2008-09-26 17:57 PDT, Darin Adler
sam: review+
Darin Adler
Comment 1 2008-09-26 17:57:43 PDT
Sam Weinig
Comment 2 2008-09-27 13:02:45 PDT
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
Comment 3 2008-09-27 15:17:02 PDT
(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
Comment 4 2008-09-27 15:18:39 PDT
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 5 2008-09-27 15:59:51 PDT
Comment on attachment 23864 [details] patch Ah, I forgot about the itemWithName case. That makes sense. r=me
Darin Adler
Comment 6 2008-09-28 02:35:34 PDT
Lucas Forschler
Comment 7 2019-02-06 09:04:16 PST
Mass moving XML DOM bugs to the "DOM" Component.
Note You need to log in before you can comment on or make changes to this bug.