Closed
Bug 711047
Opened 12 years ago
Closed 12 years ago
Remove RangeException in favor of DOMException
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: annevk, Assigned: ayg)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 2 obsolete files)
5.38 KB,
patch
|
sicking
:
review+
|
Details | Diff | Splinter Review |
25.38 KB,
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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
Assignee | ||
Comment 2•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland:-p linux64 -b o -u mochitest]
Updated•12 years ago
|
Whiteboard: [autoland:-p linux64 -b o -u mochitest] → [autoland-in-queue]
Comment 3•12 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•12 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•12 years ago
|
Whiteboard: [autoland-in-queue]
Comment 5•12 years ago
|
||
Need to update content/base/test/test_bug454326.html
Comment 6•12 years ago
|
||
Also nsRangeException, nsIDOMRangeException, NS_ERROR_MODULE_DOM_RANGE, etc. should be removed.
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland]
Updated•12 years ago
|
Whiteboard: [autoland] → [autoland-in-queue]
Comment 8•12 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
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 9•12 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•12 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+
Assignee | ||
Comment 11•12 years ago
|
||
Does this need tests and/or a change to the uuid?
Attachment #602016 -
Flags: review?(jonas)
Assignee | ||
Comment 12•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Whiteboard: [autoland]
Updated•12 years ago
|
Whiteboard: [autoland] → [autoland-in-queue]
Comment 13•12 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•12 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•12 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 18•12 years ago
|
||
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•12 years ago
|
Keywords: checkin-needed
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed83c2b26332 https://hg.mozilla.org/integration/mozilla-inbound/rev/f2ef311be88b
Keywords: checkin-needed
Comment 20•12 years ago
|
||
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
Assignee | ||
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
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
Comment 24•12 years ago
|
||
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.
Description
•