Closed Bug 711047 Opened 12 years ago Closed 12 years ago

Remove RangeException in favor of DOMException

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: annevk, Assigned: ayg)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

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.
Attached patch Patch without any test changes (obsolete) — Splinter Review
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]
Whiteboard: [autoland:-p linux64 -b o -u mochitest] → [autoland-in-queue]
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
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
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.
Attached patch Patch with tests, v1 (obsolete) — Splinter Review
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]
Whiteboard: [autoland] → [autoland-in-queue]
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
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
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+
Does this need tests and/or a change to the uuid?
Attachment #602016 - Flags: review?(jonas)
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]
Whiteboard: [autoland] → [autoland-in-queue]
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
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
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+
https://hg.mozilla.org/mozilla-central/rev/ed83c2b26332
https://hg.mozilla.org/mozilla-central/rev/f2ef311be88b
Status: ASSIGNED → RESOLVED
Closed: 12 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
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
You need to log in before you can comment on or make changes to this bug.