Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517173002:
XMLHttpRequest.abort(): follow spec wrt readyState transitions. (Closed)
DescriptionXMLHttpRequest.abort(): follow spec wrt readyState transitions.
readyState is now only set to UNSENT if abort() is called on an object
with readyState in a DONE state.
R=tyoshino,yhirano
BUG=667294
Committed: https://crrev.com/a33e6a628fd8ffb610d7ccb8ff8ad06ac04be12e
Cr-Commit-Position: refs/heads/master@{#433840}
Patch Set 1 #
Total comments: 5
Patch Set 2 : fix pass test predicate #
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sigbjornf@opera.com changed reviewers: + tyoshino@chromium.org, yhirano@chromium.org
please take a look. can't have enough of abort(), it seems.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html (right): https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:42: if ((num != 0 && this.readyState == num) || shouldn't this be num != XMLHttpRequest.DONE && this.readyState == num ? Do we need to exclude 0?
https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html (right): https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:42: if ((num != 0 && this.readyState == num) || On 2016/11/22 08:46:54, tyoshino wrote: > shouldn't this be > > num != XMLHttpRequest.DONE && this.readyState == num > > ? Do we need to exclude 0? It would be unnatural & unwanted for there to be an readystatechange event delivered when in state UNSENT, so the logic captures that.
https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html (right): https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:42: if ((num != 0 && this.readyState == num) || On 2016/11/22 08:58:33, sof wrote: > On 2016/11/22 08:46:54, tyoshino wrote: > > shouldn't this be > > > > num != XMLHttpRequest.DONE && this.readyState == num > > > > ? Do we need to exclude 0? > > It would be unnatural & unwanted for there to be an readystatechange event > delivered when in state UNSENT, so the logic captures that. Got it. But this would log PASS when num is XMLHttpRequest.DONE and this.readyState is also XMLHttpRequest.DONE, right? So, maybe we want the following: if ((num != XMLHttpRequest.UNSENT) && ((num != XMLHttpRequest.DONE && this.readyState == num) || (num == XMLHttpRequest.DONE && this.readyState == XMLHttpRequest.UNSENT)))
The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html (right): https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:42: if ((num != 0 && this.readyState == num) || On 2016/11/22 09:29:53, tyoshino wrote: > On 2016/11/22 08:58:33, sof wrote: > > On 2016/11/22 08:46:54, tyoshino wrote: > > > shouldn't this be > > > > > > num != XMLHttpRequest.DONE && this.readyState == num > > > > > > ? Do we need to exclude 0? > > > > It would be unnatural & unwanted for there to be an readystatechange event > > delivered when in state UNSENT, so the logic captures that. > > Got it. But this would log PASS when num is XMLHttpRequest.DONE and > this.readyState is also XMLHttpRequest.DONE, right? > > So, maybe we want the following: > > if ((num != XMLHttpRequest.UNSENT) && > ((num != XMLHttpRequest.DONE && this.readyState == num) || > (num == XMLHttpRequest.DONE && this.readyState == > XMLHttpRequest.UNSENT))) I've teased apart the testing of the pass condition a bit; looks ok now?
lgtm
lgtm https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html (right): https://codereview.chromium.org/2517173002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/xmlhttprequest-abort-readyState-shouldNotDispatchEvent.html:42: if ((num != 0 && this.readyState == num) || On 2016/11/22 10:14:14, sof wrote: > On 2016/11/22 09:29:53, tyoshino wrote: > > On 2016/11/22 08:58:33, sof wrote: > > > On 2016/11/22 08:46:54, tyoshino wrote: > > > > shouldn't this be > > > > > > > > num != XMLHttpRequest.DONE && this.readyState == num > > > > > > > > ? Do we need to exclude 0? > > > > > > It would be unnatural & unwanted for there to be an readystatechange event > > > delivered when in state UNSENT, so the logic captures that. > > > > Got it. But this would log PASS when num is XMLHttpRequest.DONE and > > this.readyState is also XMLHttpRequest.DONE, right? > > > > So, maybe we want the following: > > > > if ((num != XMLHttpRequest.UNSENT) && > > ((num != XMLHttpRequest.DONE && this.readyState == num) || > > (num == XMLHttpRequest.DONE && this.readyState == > > XMLHttpRequest.UNSENT))) > > I've teased apart the testing of the pass condition a bit; looks ok now? Yes. Thanks!
Description was changed from ========== XMLHttpRequest.abort(): follow spec wrt readyState transitions. readyState is now only set to UNSENT if abort() is called on an object with readyState in a DONE state. R= BUG=667294 ========== to ========== XMLHttpRequest.abort(): follow spec wrt readyState transitions. readyState is now only set to UNSENT if abort() is called on an object with readyState in a DONE state. R=tyoshino,yhirano BUG=667294 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479816820578200,
"parent_rev": "5491850bd712c0c1481980b583781d1bf2ad53e9", "commit_rev":
"282730faea8e2c766dbab9dbedd97aedb36a84d7"}
Message was sent while issue was closed.
Description was changed from ========== XMLHttpRequest.abort(): follow spec wrt readyState transitions. readyState is now only set to UNSENT if abort() is called on an object with readyState in a DONE state. R=tyoshino,yhirano BUG=667294 ========== to ========== XMLHttpRequest.abort(): follow spec wrt readyState transitions. readyState is now only set to UNSENT if abort() is called on an object with readyState in a DONE state. R=tyoshino,yhirano BUG=667294 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== XMLHttpRequest.abort(): follow spec wrt readyState transitions. readyState is now only set to UNSENT if abort() is called on an object with readyState in a DONE state. R=tyoshino,yhirano BUG=667294 ========== to ========== XMLHttpRequest.abort(): follow spec wrt readyState transitions. readyState is now only set to UNSENT if abort() is called on an object with readyState in a DONE state. R=tyoshino,yhirano BUG=667294 Committed: https://crrev.com/a33e6a628fd8ffb610d7ccb8ff8ad06ac04be12e Cr-Commit-Position: refs/heads/master@{#433840} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/a33e6a628fd8ffb610d7ccb8ff8ad06ac04be12e Cr-Commit-Position: refs/heads/master@{#433840} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
