Bug 19239 – remove half-there XML Entity DOM object support

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

I think I can get some of the speedup without removing anything. Working on that now.

[Maciej Stachowiak]

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).

[Darin Adler]

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.

[Darin Adler]

Created attachment 21336 [details]
patch, passes all regression tests

[Maciej Stachowiak]

#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.

[Maciej Stachowiak]

Comment on attachment 21336 [details]
patch, passes all regression tests

r-

[Darin Adler]

Created attachment 21343 [details]
patch that doesn't use any ifdefs

[Maciej Stachowiak]

Comment on attachment 21343 [details]
patch that doesn't use any ifdefs

r=me

[Darin Adler]

Committed revision 34138.