Summary: | remove half-there XML Entity DOM object support | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
Component: | XML | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mjs | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 17510 | ||||||||||
Attachments: |
|
Description
Darin Adler
2008-05-24 22:35:35 PDT
Created attachment 21330 [details]
patch
Here's a patch, but there are tons of regression tests that start failing with this patch.
For whatever reason, many of the dom/xhtml tests chose to use EntityReference nodes, even though they could have used any other type of node, to test functions like insertBefore, removeChild, replaceChild, etc.
There are also quite a few tests that use createEntityReference just so we can test all node types.
I think I can get some of the speedup without removing anything. Working on that now. I think you can get nearly all the speedup by moving the "have tab index" bit from Node to Element, and adding an "is read only" bit to Node (which would only be set for EntityReference nodes). In fact, I can get nearly all the speedup by just fixing the algorithm! A single virtual function call is quite affordable. The old algorithm instead did <depth*2 + 1> virtual function calls. That was the real problem. Patch coming shortly. Moving from a virtual function to a bit will make things even faster, but I don't think we'll see this function on the profile once my patch is in. Created attachment 21336 [details]
patch, passes all regression tests
#ifdef SUPPORT_ENTITY_NODES ... shouldn't this be ENABLE(ENTITY_NODES)? Also, given this: +inline bool Node::isReadOnlyNode() const +{ + return nodeType() == ENTITY_REFERENCE_NODE; +} I don't understand why the other branch of the ifdef has overrides of the non-virtual isReadOnlyNode method. I'm also not sure an ifdef is the best approach here. I don't think we will ever want to support two different entity modes, so if we expand support, we'll just want to change the code, not flip the value of a #define. I suggest just doing it without an ifdef and leaving a comment on what would be needed if we supported entity nodes and unexpanded entity references. Comment on attachment 21336 [details]
patch, passes all regression tests
r-
Created attachment 21343 [details]
patch that doesn't use any ifdefs
Comment on attachment 21343 [details]
patch that doesn't use any ifdefs
r=me
Committed revision 34138. |