Bug 19239 - remove half-there XML Entity DOM object support
Summary: remove half-there XML Entity DOM object support
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Darin Adler
Depends on:
Blocks: 17510
  Show dependency treegraph
Reported: 2008-05-24 22:35 PDT by Darin Adler
Modified: 2008-05-26 13:08 PDT (History)
1 user (show)

See Also:

patch (16.65 KB, patch)
2008-05-24 23:44 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
patch, passes all regression tests (17.17 KB, patch)
2008-05-25 09:22 PDT, Darin Adler
mjs: review-
Details | Formatted Diff | Diff
patch that doesn't use any ifdefs (22.81 KB, patch)
2008-05-25 19:35 PDT, Darin Adler
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2008-05-24 22:35:35 PDT
Currently, there is a bit of XML Entity support, but not enough to be useful. The actual entities are expanded when encountered, which is good. But we also have an Entity DOM class, which is never instantiated, and an EntityReferece DOM class, which is instantiated only by createEntityReference, but never has any children, so is useless.

For this bug I will change createEntityReference so it will throw NOT_SUPPORTED_ERR for XML documents, just as it does for HTML documents. I think this will create no practical difficulty, since it does nothing useful today, and is highly unlikely to be widely used on the web.

Once this is done, we can guarantee there are no Entity or EntityReference nodes, and hence no read-only nodes, at all in WebKit, so we can remove some relatively costly checks in the code.

Some day if we find a reason and want to bring these node types back, we can find an efficient way to do so.
Comment 1 Darin Adler 2008-05-24 23:44:24 PDT
Created attachment 21330 [details]

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.
Comment 2 Darin Adler 2008-05-25 01:22:57 PDT
I think I can get some of the speedup without removing anything. Working on that now.
Comment 3 Maciej Stachowiak 2008-05-25 01:42:18 PDT
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).
Comment 4 Darin Adler 2008-05-25 08:28:30 PDT
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.
Comment 5 Darin Adler 2008-05-25 09:22:22 PDT
Created attachment 21336 [details]
patch, passes all regression tests
Comment 6 Maciej Stachowiak 2008-05-25 14:21:55 PDT
#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 7 Maciej Stachowiak 2008-05-25 14:22:50 PDT
Comment on attachment 21336 [details]
patch, passes all regression tests

Comment 8 Darin Adler 2008-05-25 19:35:35 PDT
Created attachment 21343 [details]
patch that doesn't use any ifdefs
Comment 9 Maciej Stachowiak 2008-05-25 20:14:09 PDT
Comment on attachment 21343 [details]
patch that doesn't use any ifdefs

Comment 10 Darin Adler 2008-05-26 13:08:35 PDT
Committed revision 34138.