Last Comment Bug 711047 - Remove RangeException in favor of DOMException
: Remove RangeException in favor of DOMException
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-15 06:54 PST by Anne (:annevk)
Modified: 2012-04-29 16:47 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch without any test changes (9.22 KB, patch)
2012-02-24 11:41 PST, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Review
Patch with tests, v1 (29.05 KB, patch)
2012-02-26 11:49 PST, :Aryeh Gregor (away until August 15)
jonas: review+
Details | Diff | Review
Patch part 1, v1 -- Remove raises() from nsIDOMRange (5.38 KB, patch)
2012-03-01 10:37 PST, :Aryeh Gregor (away until August 15)
jonas: review+
Details | Diff | Review
Patch part 2, v2 -- Remove RangeException in favor of DOMException (25.38 KB, patch)
2012-03-01 10:38 PST, :Aryeh Gregor (away until August 15)
Ms2ger: review+
Details | Diff | Review

Description Anne (:annevk) 2011-12-15 06:54:34 PST
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.
Comment 2 :Aryeh Gregor (away until August 15) 2012-02-24 11:41:37 PST
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.
Comment 3 Mozilla RelEng Bot 2012-02-24 11:46:32 PST
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 Mozilla RelEng Bot 2012-02-24 14:02:47 PST
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
Comment 5 :Ms2ger 2012-02-25 01:50:49 PST
Need to update content/base/test/test_bug454326.html
Comment 6 Masatoshi Kimura [:emk] 2012-02-25 11:43:58 PST
Also nsRangeException, nsIDOMRangeException, NS_ERROR_MODULE_DOM_RANGE, etc. should be removed.
Comment 7 :Aryeh Gregor (away until August 15) 2012-02-26 11:49:17 PST
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.
Comment 8 Mozilla RelEng Bot 2012-02-26 11:52:45 PST
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
Comment 9 Mozilla RelEng Bot 2012-02-26 15:46:09 PST
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
Comment 10 Jonas Sicking (:sicking) 2012-02-26 21:03:01 PST
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.
Comment 11 :Aryeh Gregor (away until August 15) 2012-03-01 10:37:14 PST
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?
Comment 12 :Aryeh Gregor (away until August 15) 2012-03-01 10:38:27 PST
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.)
Comment 13 Mozilla RelEng Bot 2012-03-01 10:44:28 PST
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 Mozilla RelEng Bot 2012-03-01 15:16:35 PST
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
Comment 15 Jonas Sicking (:sicking) 2012-03-03 14:15:38 PST
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.
Comment 16 Jonas Sicking (:sicking) 2012-03-03 14:17:58 PST
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 17 Jonas Sicking (:sicking) 2012-03-03 14:20:38 PST
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
Comment 18 :Ms2ger 2012-03-03 14:26:14 PST
Comment on attachment 602018 [details] [diff] [review]
Patch part 2, v2 -- Remove RangeException in favor of DOMException

Indeed it is.
Comment 21 :Aryeh Gregor (away until August 15) 2012-03-05 09:09:40 PST
(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.
Comment 22 Daniel Holbert [:dholbert] 2012-03-16 15:19:26 PDT
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
Comment 23 Phil Ringnalda (:philor) 2012-03-17 17:32:17 PDT
https://hg.mozilla.org/mozilla-central/rev/cbcba94ec2c9
Comment 24 Florian Scholz [:fscholz] (MDN) 2012-04-29 16:47:51 PDT
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

Note You need to log in before you can comment on or make changes to this bug.