Bug 91316 – Remove ENTITY_REFERENCE_NODE from WebCore

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Remove ENTITY_REFERENCE_NODE from WebCore

[Eric Seidel (no email)]

Created attachment 152416 [details]
wip

[Eric Seidel (no email)]

This will cause a zillion w3c tests to fail.  It's also unclear to me exactly what balance to strike.

Currently I'm removing all mentions of EntityReference, except for Node.ENTITY_REFERENCE_NODE in JavaScript (it felt odd to remove the constant).

Other things which I removed, but we might not chose to:
window.EntityReference
document.createEntityReference

WebKit API i removed:
WebNode::EntityReferenceNode (chromium)
@DOMEntityReference (Mac API)
-[DOMDocument createEntityReference] (Mac API)

I also removed the Windows DOM bindings, but since those weren't even implemented, I can't imagine that matters.

To give context: it was *only* possible to create EntityReference Nodes inside an XML document, using the javascxript document.createEntityReference call.  In HTML, that would just throw an exception.  And the parser would never create these nodes.

This ended up simplifying a whole bunch of Node and Range code. :)

I'm interested in thoughts from the Mac API and Chromium API folks.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Please wait for approval from [e-mail] (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

[Gustavo Noronha (kov)]

Comment on attachment 152416 [details]
wip

Attachment 152416 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13243383

[WebKit Review Bot]

Comment on attachment 152416 [details]
wip

Attachment 152416 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13235414

[Build Bot]

Comment on attachment 152416 [details]
wip

Attachment 152416 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13221982

[Ojan Vafai]

(In reply to comment #2)
> This will cause a zillion w3c tests to fail.  It's also unclear to me exactly what balance to strike.

Yes, this is expected. We just need to rebaseline them. All of your changes look correct to me. I don't know about the Mac API bits, but otherwise, if the ChangeLog had a description and a link to the DOM4 spec, I'd be ready to r+.

> This ended up simplifying a whole bunch of Node and Range code. :)

That's the idea! As you can imagine, it simplifies the DOM spec a lot as well.

[Ojan Vafai]

Also, looks like you have compile errors on some platforms. Looking at the chromium case, I think you just need to remove EntityReference.idl from a few more places. 

http://code.google.com/p/chromium/source/search?q=EntityReference.idl&origq=EntityReference.idl&btnG=Search+Trunk

[Eric Seidel (no email)]

Created attachment 152627 [details]
still wip

[WebKit Review Bot]

Comment on attachment 152627 [details]
still wip

Attachment 152627 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13265145

New failing tests:
dom/xhtml/level1/core/hc_nodevalue03.xhtml
dom/xhtml/level1/core/hc_attrgetvalue2.xhtml
dom/xhtml/level3/core/noderemovechild13.xhtml
dom/xhtml/level3/core/documentadoptnode16.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition28.xhtml
dom/xhtml/level3/core/nodereplacechild19.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition27.xhtml
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref.html
dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg
dom/xhtml/level3/core/noderemovechild14.xhtml
dom/xhtml/level3/core/documentadoptnode06.xhtml
dom/xhtml/level3/core/noderemovechild12.xhtml
dom/xhtml/level3/core/nodeinsertbefore15.xhtml
dom/svg/level3/xpath/XPathExpression_evaluate_NOT_SUPPORTED_ERR.svg
dom/html/level1/core/hc_attrgetvalue2.html
dom/html/level1/core/documentinvalidcharacterexceptioncreateentref1.html
dom/xhtml/level3/core/nodeinsertbefore11.xhtml
dom/xhtml/level3/core/nodereplacechild18.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition29.xhtml
css3/filters/blur-filter-page-scroll-self.html
dom/xhtml/level3/core/nodecomparedocumentposition26.xhtml
dom/xhtml/level3/core/noderemovechild15.xhtml
dom/xhtml/level3/core/nodereplacechild22.xhtml
dom/html/level1/core/hc_nodevalue03.html
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref1.xhtml
dom/xhtml/level3/core/documentrenamenode27.xhtml
dom/xhtml/level3/core/nodegettextcontent17.xhtml
dom/xhtml/level1/core/documentinvalidcharacterexceptioncreateentref.xhtml

[WebKit Review Bot]

Created attachment 152669 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Eric Seidel (no email)]

Created attachment 152802 [details]
Patch

[Ojan Vafai]

Comment on attachment 152802 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152802&action=review

r=me assuming you make the test changes and all the EWS bots go green. Thanks for doing this!

> Source/WebKit/chromium/public/WebNode.h:77
> +        // EntityReferenceNodes are deprecated and impossible possible to create in WebKit.

s/possible //

> LayoutTests/fast/dom/Window/get-set-properties-expected.txt:38
> +*** FAIL: canGet('EntityReference') should be 'true' but instead is false. ***

Can you just delete this line from the test?

> LayoutTests/fast/dom/dom-constructors-expected.txt:15
> +FAIL TryAllocate('EntityReference') should be exception. Was no constructor.

ditto

> LayoutTests/fast/dom/wrapper-classes-expected.txt:52
> +FAIL jsWrapperClass(xmlDocument.createEntityReference()) should be EntityReference. Threw exception TypeError: 'undefined' is not a function (evaluating 'xmlDocument.createEntityReference()')
> +FAIL jsWrapperClass(xmlDocument.createEntityReference().__proto__) should be EntityReferencePrototype. Threw exception TypeError: 'undefined' is not a function (evaluating 'xmlDocument.createEntityReference()')
> +FAIL jsWrapperClass(xmlDocument.createEntityReference().constructor) should be EntityReferenceConstructor. Threw exception TypeError: 'undefined' is not a function (evaluating 'xmlDocument.createEntityReference()')

ditto

[Adam Barth]

WebKit API change LGTM

[WebKit Review Bot]

Comment on attachment 152802 [details]
Patch

Attachment 152802 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13271303

New failing tests:
dom/xhtml/level1/core/hc_nodevalue03.xhtml
dom/xhtml/level1/core/hc_attrgetvalue2.xhtml
dom/xhtml/level3/core/noderemovechild13.xhtml
dom/xhtml/level3/core/documentadoptnode16.xhtml
dom/xhtml/level3/core/nodereplacechild33.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition28.xhtml
dom/xhtml/level3/core/nodereplacechild19.xhtml
dom/xhtml/level3/core/nodereplacechild37.xhtml
dom/xhtml/level3/core/nodereplacechild35.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition27.xhtml
dom/svg/level3/xpath/XPathEvaluator_evaluate_NOT_SUPPORTED_ERR.svg
dom/xhtml/level3/core/noderemovechild14.xhtml
dom/xhtml/level3/core/documentadoptnode06.xhtml
dom/xhtml/level3/core/noderemovechild12.xhtml
dom/xhtml/level3/core/nodereplacechild34.xhtml
dom/xhtml/level3/core/nodeinsertbefore15.xhtml
dom/svg/level3/xpath/XPathExpression_evaluate_NOT_SUPPORTED_ERR.svg
dom/xhtml/level3/core/nodereplacechild36.xhtml
dom/xhtml/level3/core/nodeinsertbefore11.xhtml
dom/xhtml/level3/core/nodereplacechild18.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition29.xhtml
dom/xhtml/level3/core/nodecomparedocumentposition26.xhtml
dom/xhtml/level3/core/noderemovechild15.xhtml
dom/xhtml/level3/core/nodereplacechild30.xhtml
dom/xhtml/level3/core/nodereplacechild22.xhtml
dom/xhtml/level3/core/documentrenamenode27.xhtml
dom/xhtml/level3/core/nodegettextcontent17.xhtml
dom/xhtml/level3/core/nodereplacechild23.xhtml

[WebKit Review Bot]

Created attachment 152827 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Alexey Proskuryakov]

I don't think that we're allowed to remove Objective C APIs even if we're confident that impact would be negligible, are we? Please don't land without approval from Tim Hatcher or Darin Adler.

[Eric Seidel (no email)]

I'm happy to ignore the Obj-c parts of the change, and leave the dead objects in if that would be better for Mac.  Tim's been CC'd since the start.  I welcome any and all input from deciders at Apple.

[Timothy Hatcher]

Yes, Alexey is right. The ObjC API needs to stay. It can be a dead object with the same header.

[Alexey Proskuryakov]

Comment on attachment 152802 [details]
Patch

Marking r- per the above.

[Anders Carlsson]

Looks like this has been done now.