Closed
Bug 1311798
Opened 8 years ago
Closed 8 years ago
[XHR2] Align XHR abort() with the spec.
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: wisniewskit, Assigned: wisniewskit)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
The web platform tests for XMLHttpRequest abort() are presently being updated to better-match the spec text [1, 2]. This patch updates Firefox accordingly so that the revised tests will pass. Try seems fine with it [3]. Note that I'm not 100% sure if it would be better to make both aOptionalException and aRv pointers or Optionals<> in the new RequestErrorSteps method, rather than using NS_OK as a sentinel value for "do not throw an exception". Thoughts? Also note that the same changes to the web platform tests in this patch are already being merged remotely [4], so there is no need to review them here. Once we're ready to land this patch, I will revise it appropriately to avoid any merge conflicts on our end. [1] https://github.com/whatwg/xhr/issues/88 [2] https://xhr.spec.whatwg.org/#the-abort()-method [3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1183408163bc88f1b172f7cc7e502fca8800490 [4] https://github.com/w3c/web-platform-tests/pull/4033
Assignee | ||
Comment 1•8 years ago
|
||
(adding the patch, as Bugzilla ate it in my original post).
Attachment #8803065 -
Flags: review?(amarchesini)
Comment 2•8 years ago
|
||
FYI, baku is on vacation.
Assignee | ||
Comment 3•8 years ago
|
||
Thanks. Would you mind doing the honors? I don't think there's any particular rush, but I also don't think it needs baku's attention specifically.
Flags: needinfo?(bugs)
Comment 5•8 years ago
|
||
(::Abort(ErrorResult& arv) odd naming for the argument. Should be aRv.) Given how the spec is written, I think the approach you've taken is good enough. Using Optional<> wouldn't really give us anything, I think. But don't compare to NS_OK, but do NS_FAILED(aOptionalException)
Flags: needinfo?(bugs)
Comment 6•8 years ago
|
||
Comment on attachment 8803065 [details] [diff] [review] update_xhr_abort_method_to_match_spec_requirements.diff Review of attachment 8803065 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xhr/XMLHttpRequestMainThread.cpp @@ +1065,5 @@ > > void > +XMLHttpRequestMainThread::RequestErrorSteps(const ProgressEventType aEventType, > + const nsresult aOptionalException, > + ErrorResult& aRv) { { in a new line. @@ +1115,1 @@ > XMLHttpRequestMainThread::Abort(ErrorResult& arv) rename this to aRv
Attachment #8803065 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Thanks, baku! Review comments are addressed in this version, and a fresh try run is fine [1], so I'm carrying over r+ and requesting checkin. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=3cc69d558e457e23d58cdb2ebfa29d2e4cec58be
Attachment #8803065 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3421c306d8cf Align XMLHttpRequest.abort() with the spec. r=baku
Keywords: checkin-needed
Comment 9•8 years ago
|
||
Backed out for failing event-readystatechange-loaded.htm and unexpected passes: https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7e3bcf119a09e85bf221de52b9e84016c0e0d1 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3421c306d8cf844b149594517340275173a5460f Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=39519224&repo=mozilla-inbound TEST-UNEXPECTED-PASS | /XMLHttpRequest/abort-during-open.htm | XMLHttpRequest: abort() during OPEN - expected FAIL TEST-UNEXPECTED-PASS | /XMLHttpRequest/abort-during-open.worker.html | Untitled - {} TEST-UNEXPECTED-PASS | /XMLHttpRequest/abort-event-abort.htm | XMLHttpRequest: The abort() method: do not fire abort event in OPENED state when send() flag is unset. - expected FAIL TEST-UNEXPECTED-FAIL | /XMLHttpRequest/event-readystatechange-loaded.htm | XMLHttpRequest: the LOADING state change may be emitted multiple times - assert_equals: LOADING state change TEST-UNEXPECTED-PASS | /XMLHttpRequest/send-data-unexpected-tostring.htm | abort() called from data stringification - expected FAIL
Flags: needinfo?(wisniewskit)
Assignee | ||
Comment 10•8 years ago
|
||
Weird, I must have had a brain-fart and forgotten to mark those tests as passing. Here's a version rebased to inbound that does that. I can't replicate the failure, however; I wonder if it was a hiccup? Let's try again and see...
Attachment #8812586 -
Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
Yes, the other jobs don't have that failure.
Comment 12•8 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8475c0038c Align XMLHttpRequest.abort() with the spec. r=baku
Keywords: checkin-needed
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ca8475c0038c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 14•7 years ago
|
||
I guess we want this to ride the train with 53.
You need to log in
before you can comment on or make changes to this bug.
Description
•