Bug 93009 – Range::isPointInRange incorrectly throws WRONG_DOCUMENT_ERR

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Emil A Eklund]

The latest working draft of the DOM4 spec has all but killed the WRONG_DOCUMENT_ERR exception. Range::compareBoundaryPoints and Range::comparePoint are the only two remaining uses for it. As such, update isPointInRange to return false instead of throwing an exception when the range and point are in different documents.

This matches the Mozilla behavior.

[Emil A Eklund]

Created attachment 156122 [details]
Patch

[Ojan Vafai]

Comment on attachment 156122 [details]
Patch

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

> LayoutTests/fast/html/range-point-in-range-for-different-documents.html:31
> +                if (range.isPointInRange(testNode, 1))
> +                    testFailed('isPointInRange returned true.');
> +                else
> +                    testPassed('isPointInRange returned false.');

Nit: this could be:
shouldBeFalse("range.isPointInRange(testNode, 1)")

[Alexey Proskuryakov]

Comment on attachment 156122 [details]
Patch

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

> Source/WebCore/dom/Range.cpp:307
> +    // The DOM4 working draft no longer has these cases throw a
> +    // WRONG_DOCUMENT_ERR exception. This matches the behavior of Mozilla.

I don't think that this comment is helpful. A lot of our code is based on research like this, and the best way to figure out the reasons for some code is to change it, and see which tests break.

Such comments are going to get obsolete - we wouldn't be happy if core code was all covered by references to IE 4.

[Emil A Eklund]

(In reply to comment #2)
> Nit: this could be:
> shouldBeFalse("range.isPointInRange(testNode, 1)")

Good idea.


(In reply to comment #3)
> I don't think that this comment is helpful. A lot of our code is based on research like this, and the best way to figure out the reasons for some code is to change it, and see which tests break.
> 
> Such comments are going to get obsolete - we wouldn't be happy if core code was all covered by references to IE 4.

You have a point there, I'll remove the comment.

Thank you.

[Emil A Eklund]

Created attachment 156145 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 156145 [details]
Patch for landing

Rejecting attachment 156145 [details] from commit-queue.

New failing tests:
fast/dom/move-nodes-across-documents.html
Full output: http://queues.webkit.org/results/13430102

[WebKit Review Bot]

Created attachment 156163 [details]
Archive of layout-test-results from gce-cq-02

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

[Emil A Eklund]

Committed r124506: <http://trac.webkit.org/changeset/124506>