The default bug view has changed. See this FAQ.

Remove RangeException in favor of DOMException

RESOLVED FIXED in mozilla13

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: annevk, Assigned: ayg)

Tracking

({dev-doc-complete})

Trunk
mozilla13
x86
Mac OS X
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
See http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html for more details. Basically everything should use DOMException as discussed on public-webapps@w3.org.
Causes test failures in at least the following files:

http://dvcs.w3.org/hg/editing/raw-file/tip/selecttest/collapse.html
http://dvcs.w3.org/hg/editing/raw-file/tip/selecttest/extend.html
http://dvcs.w3.org/hg/editing/raw-file/tip/selecttest/selectAllChildren.html

http://w3c-test.org/webapps/DOMCore/tests/approved/Range-selectNode.html
http://w3c-test.org/webapps/DOMCore/tests/approved/Range-set.html
http://w3c-test.org/webapps/DOMCore/tests/approved/Range-surroundContents.html
Created attachment 600466 [details] [diff] [review]
Patch without any test changes

This rather trivial patch fixes the problems.  It hopefully causes test failures -- anything else would suggest our Range coverage is rather poor!  But the tryserver is more convenient for tracking down failures than localhost, so I'll use that.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Whiteboard: [autoland:-p linux64 -b o -u mochitest]

Updated

5 years ago
Whiteboard: [autoland:-p linux64 -b o -u mochitest] → [autoland-in-queue]

Comment 3

5 years ago
Autoland Patchset:
	Patches: 600466
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=7e43c855bc72
Try run started, revision 7e43c855bc72. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=7e43c855bc72

Comment 4

5 years ago
Try run for 7e43c855bc72 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7e43c855bc72
Results (out of 7 total builds):
    success: 6
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-7e43c855bc72

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Need to update content/base/test/test_bug454326.html
Also nsRangeException, nsIDOMRangeException, NS_ERROR_MODULE_DOM_RANGE, etc. should be removed.
Created attachment 600794 [details] [diff] [review]
Patch with tests, v1

This now removes all the stuff mentioned in comment 6.  I grepped for 'range.*exception' and removed anything that hit, pretty much.  It fixes one existing test as noted in comment 5, and adds a new one that just checks !"RangeException" in window.  Detailed tests on what exactly throws DOMException and which type it throws will come with bug 647323, and we didn't have them before, so I don't think we need to test them here.

content/base/test/test_bug498240.html mentions RangeException, but the function that tests for it is commented out.  I left it alone, but I can fix it if desired.

In xpcom/base/nsError.h, I left a comment saying what 23 used to be so people don't get confused by the gap in the sequence.
Attachment #600466 - Attachment is obsolete: true
Attachment #600794 - Flags: review?(jonas)
Whiteboard: [autoland]

Updated

5 years ago
Whiteboard: [autoland] → [autoland-in-queue]

Comment 8

5 years ago
Autoland Patchset:
	Patches: 600794
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=5d2398001594
Try run started, revision 5d2398001594. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=5d2398001594
Keywords: dev-doc-needed

Comment 9

5 years ago
Try run for 5d2398001594 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5d2398001594
Results (out of 216 total builds):
    success: 177
    warnings: 24
    failure: 15
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-5d2398001594

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Comment on attachment 600794 [details] [diff] [review]
Patch with tests, v1

Review of attachment 600794 [details] [diff] [review]:
-----------------------------------------------------------------

Yay!!

::: content/base/test/test_bug454326.html
@@ +84,5 @@
>        is(r3.toString(), "Hello ", "Wrong range!");
>      } catch(e) {
>        ex = e;
> +	  is(Object.getPrototypeOf(e), DOMException.prototype, "Didn't get DOMException!");
> +      is(e.code, 11, "Didn't get INVALID_STATE_ERR exception!");

Fix indentation. Tabs?

@@ +100,5 @@
>        is(r3.toString(), "World!", "Wrong range!");
>      } catch(e) {
>        ex = e;
> +	  is(Object.getPrototypeOf(e), DOMException.prototype, "Didn't get DOMException!");
> +      is(e.code, 11, "Didn't get INVALID_STATE_ERR exception!");

Same here.

@@ +116,5 @@
>        is(r4.toString(), "Hello World!", "Wrong range!");
>      } catch(e) {
>        ex = e;
> +	  is(Object.getPrototypeOf(e), DOMException.prototype, "Didn't get DOMException!");
> +      is(e.code, 11, "Didn't get INVALID_STATE_ERR exception!");

And here.

::: dom/interfaces/range/nsIDOMRange.idl
@@ +67,5 @@
>    readonly attribute nsIDOMNode       commonAncestorContainer;
>                                          // raises(DOMException) on retrieval
>  
>    void               setStart(in nsIDOMNode refNode, in long offset)
> +                                        raises(DOMException);

Feel free to just remove this raises stuff entirely.
Attachment #600794 - Flags: review?(jonas) → review+
Created attachment 602016 [details] [diff] [review]
Patch part 1, v1 -- Remove raises() from nsIDOMRange

Does this need tests and/or a change to the uuid?
Attachment #602016 - Flags: review?(jonas)
Created attachment 602018 [details] [diff] [review]
Patch part 2, v2 -- Remove RangeException in favor of DOMException

With whitespace fixes.  (I need to configure vim to change indent styling per-directory -- the headers fix it for actual source code, but tests don't have them.)
Attachment #600794 - Attachment is obsolete: true
Attachment #602018 - Flags: review?(jonas)
Whiteboard: [autoland]

Updated

5 years ago
Whiteboard: [autoland] → [autoland-in-queue]

Comment 13

5 years ago
Autoland Patchset:
	Patches: 602016, 602018
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=d82b93cc12d2
Try run started, revision d82b93cc12d2. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=d82b93cc12d2

Comment 14

5 years ago
Try run for d82b93cc12d2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d82b93cc12d2
Results (out of 216 total builds):
    exception: 2
    success: 175
    warnings: 37
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-d82b93cc12d2

Updated

5 years ago
Whiteboard: [autoland-in-queue]
Arg, please don't ask review for a big patch and when you get it re-ask for the same patch with a small changes :(

Please attach a interdiff from the patch that was reviewed, or explain in detail what changed.
Or did you simply fix the review comments? If so it helps to say so, though when someone says "fix X, r=me" that means that you don't need to re-ask for review if you just make the requested changes.
Comment on attachment 602016 [details] [diff] [review]
Patch part 1, v1 -- Remove raises() from nsIDOMRange

Review of attachment 602016 [details] [diff] [review]:
-----------------------------------------------------------------

This part looks good though, so if the other patch is just review fixes then r=me on both of them
Attachment #602016 - Flags: review?(jonas) → review+
Comment on attachment 602018 [details] [diff] [review]
Patch part 2, v2 -- Remove RangeException in favor of DOMException

Indeed it is.
Attachment #602018 - Flags: review?(jonas) → review+

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed83c2b26332
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ef311be88b
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed83c2b26332
https://hg.mozilla.org/mozilla-central/rev/f2ef311be88b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
(In reply to Jonas Sicking (:sicking) from comment #15)
> Arg, please don't ask review for a big patch and when you get it re-ask for
> the same patch with a small changes :(
> 
> Please attach a interdiff from the patch that was reviewed, or explain in
> detail what changed.

Sorry that I wasn't clear -- "With whitespace fixes" was supposed to mean that the only change was fixing the whitespace as you requested.

(In reply to Jonas Sicking (:sicking) from comment #16)
> Or did you simply fix the review comments? If so it helps to say so, though
> when someone says "fix X, r=me" that means that you don't need to re-ask for
> review if you just make the requested changes.

Okay, thanks for the clarification.  I wasn't totally sure and thought I'd play it safe, since other times people have explicitly said "r=me *if* you fix this" and I wasn't totally sure that's what you meant.
Landed a comment-only followup to part 2, to fix a build warning about C++ comments encountered while compiling .c files: https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcba94ec2c9
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/cbcba94ec2c9
Can't find any occurrences of RangeException on MDN, which would be in need to be clarified.
So, this got mentioned on
https://developer.mozilla.org/en/Firefox_13_for_developers#DOM
https://developer.mozilla.org/en/DOM/range#Exceptions
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.