Clean up XHR API #9383

KiChjang · 2016-01-20T04:42:17Z

I've also added annotations about the steps that we're performing within each method.

highfive · 2016-01-20T04:42:20Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

frewsxcv · 2016-01-20T05:05:59Z

components/script/dom/xmlhttprequest.rs

                }
            },
+            // Step 1


Move this above since it is step 1

Nevermind, not trivial since it's matching on _

Ms2ger · 2016-01-20T10:05:23Z

How about a sync_allowed method that checks the global and the sync flag?

jdm · 2016-01-20T15:29:12Z

components/script/dom/xmlhttprequest.rs

@@ -295,6 +295,9 @@ impl XMLHttpRequestMethods for XMLHttpRequest {

    // https://xhr.spec.whatwg.org/#the-open()-method
    fn Open(&self, method: ByteString, url: USVString) -> ErrorResult {
+        // Step 1
+        if !self.global().r().as_window().Document().r().is_fully_active() { return Err(Error::InvalidState); }


This will cause a panic when calling this method in a worker.

If settings object's responsible document is not fully active, throw an InvalidStateError exception.

Hmm... how would we get a responsible document from a Worker, in that case?

That concept is not implemented yet. We also will not be able to interact with the Document on the worker thread; we would need to block on a runnable sent to the document's associated script thread.

KiChjang · 2016-01-20T19:43:14Z

@Ms2ger Done.

KiChjang · 2016-01-27T00:10:10Z

r? @Ms2ger

Ms2ger · 2016-02-01T13:55:40Z

Looking at this again, sync_allowed doesn't really make sense as a method name. Maybe sync_in_window? Feels like we should be able to do better.

KiChjang · 2016-02-01T14:40:40Z

Done that as well.

Ms2ger · 2016-02-04T11:36:06Z

You made me read through most of the XHR spec, so now you get to file issues for all the bugs I found :)

-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

components/script/dom/xmlhttprequest.rs, line 197 [r3] (raw file):
Now the value you're returning is backwards :)

components/script/dom/xmlhttprequest.rs, line 308 [r3] (raw file):
Not entirely sure this is useful, since it boils down to browsing_context.active_document() == browsing_context.active_document() right now.

components/script/dom/xmlhttprequest.rs, line 310 [r3] (raw file):
This issue has been fixed; remove the reference.

components/script/dom/xmlhttprequest.rs, line 329 [r3] (raw file):
This is no longer step 2.

components/script/dom/xmlhttprequest.rs, line 347 [r3] (raw file):
Please file an issue and point to it here.

components/script/dom/xmlhttprequest.rs, line 388 [r3] (raw file):
We don't seem to do step 3; please file an issue and add a comment here.

components/script/dom/xmlhttprequest.rs, line 416 [r3] (raw file):
What step is this? Is it reachable? If so, do we have tests that reach it?

components/script/dom/xmlhttprequest.rs, line 436 [r3] (raw file):
Pointless to_vec call; file a followup (needs an API on ByteString ).

components/script/dom/xmlhttprequest.rs, line 448 [r3] (raw file):
This fix should cause a test to pass; if not, write one.

Also, wrap into three lines.

components/script/dom/xmlhttprequest.rs, line 451 [r3] (raw file):
Add a pointer to the bit of the spec where this is handled.

components/script/dom/xmlhttprequest.rs, line 483 [r3] (raw file):
Split into two separate match arms for symmetry.

components/script/dom/xmlhttprequest.rs, line 523 [r3] (raw file):
So we didn't do this in the sync case before; check if there's a way to test that.

components/script/dom/xmlhttprequest.rs, line 698 [r3] (raw file):
This seems to ignore "excluding any parameters, and converted to ASCII lowercase." Followup.

components/script/dom/xmlhttprequest.rs, line 702 [r3] (raw file):
Weird indentation.

components/script/dom/xmlhttprequest.rs, line 724 [r3] (raw file):
We need to not do this in workers; followup.

Also not sure of the point of doing this in the match rather than with a plain if below the match.

components/script/dom/xmlhttprequest.rs, line 755 [r3] (raw file):
Followup to move this into a function of its own.

Comments from the review on Reviewable.io

KiChjang · 2016-02-05T11:25:08Z

Review status: 0 of 1 files reviewed at latest revision, 17 unresolved discussions.

components/script/dom/xmlhttprequest.rs, line 308 [r3] (raw file):
This sounds more like a follow-up issue to fix the Document method. I'd say that we should still keep it here since the spec says so.

components/script/dom/xmlhttprequest.rs, line 416 [r3] (raw file):
This should actually be marked unreachable!(). The .as_str() method returns Some(&str) if the ByteString is encoded using UTF-8, and None otherwise.

components/script/dom/xmlhttprequest.rs, line 436 [r3] (raw file):
http://hyper.rs/hyper/hyper/header/struct.Headers.html#method.set_raw seems to accept a Vec<Vec<u8>>.

components/script/dom/xmlhttprequest.rs, line 451 [r3] (raw file):
I don't actually know which part of the spec this algorithm refers to.

components/script/dom/xmlhttprequest.rs, line 755 [r3] (raw file):
Yeah... all of the Response Document/JSON/ArrayBuffer/Blob methods need to be implemented.

Comments from the review on Reviewable.io

KiChjang · 2016-02-05T11:27:59Z

Review status: 0 of 1 files reviewed at latest revision, 17 unresolved discussions.

components/script/dom/xmlhttprequest.rs, line 523 [r3] (raw file):
This can be tested on a few places - calling the Send method again for example, or SetRequestHeader, SetWithCredentials and Abort.

Comments from the review on Reviewable.io

bors-servo · 2016-02-07T00:11:46Z

☔ The latest upstream changes (presumably #9562) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-02-18T14:53:50Z

⌛ Testing commit 066c5ea with merge 01bc4e6...


        Auto merge of #9383 - KiChjang:xhr-cleanup, r=Ms2ger

Clean up XHR API I've also added annotations about the steps that we're performing within each method.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9383)

bors-servo · 2016-02-18T15:41:26Z

💔 Test failed - linux-rel

jdm · 2016-02-18T15:59:25Z

@bors-servo: retry

#9488

bors-servo · 2016-02-18T15:59:26Z

⚡ Previous build results for android, gonk, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-rel...

bors-servo · 2016-02-18T16:56:18Z

💔 Test failed - linux-rel

KiChjang · 2016-02-18T17:58:49Z

@bors-servo retry

#9092

bors-servo · 2016-02-18T18:36:05Z

⌛ Testing commit 066c5ea with merge 911e9ca...


        Auto merge of #9383 - KiChjang:xhr-cleanup, r=Ms2ger

Clean up XHR API I've also added annotations about the steps that we're performing within each method.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9383)

bors-servo · 2016-02-18T19:24:04Z

💔 Test failed - mac-rel-wpt

KiChjang · 2016-02-18T19:27:10Z

@bors-servo retry

#8120

bors-servo · 2016-02-18T20:55:19Z

⌛ Testing commit 066c5ea with merge 96d1853...


        Auto merge of #9383 - KiChjang:xhr-cleanup, r=Ms2ger

Clean up XHR API I've also added annotations about the steps that we're performing within each method.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9383)

bors-servo · 2016-02-18T21:44:27Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Jan 20, 2016

KiChjang force-pushed the KiChjang:xhr-cleanup branch from 0470ffe to 83f5d06 Jan 20, 2016

frewsxcv reviewed Jan 20, 2016
View changes

KiChjang force-pushed the KiChjang:xhr-cleanup branch 3 times, most recently from c1c49dc to 5932923 Jan 20, 2016

KiChjang force-pushed the KiChjang:xhr-cleanup branch from 5932923 to a3fb02e Jan 20, 2016

jdm reviewed Jan 20, 2016
View changes

KiChjang force-pushed the KiChjang:xhr-cleanup branch 3 times, most recently from aacaf70 to 1771bb4 Jan 20, 2016

highfive assigned Ms2ger Jan 27, 2016

KiChjang force-pushed the KiChjang:xhr-cleanup branch from 1771bb4 to 7c5295e Feb 1, 2016

Ms2ger added S-needs-code-changes and removed S-awaiting-review labels Feb 4, 2016

KiChjang force-pushed the KiChjang:xhr-cleanup branch from 7c5295e to 6f40abd Feb 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 5, 2016

KiChjang force-pushed the KiChjang:xhr-cleanup branch from 6f40abd to ccc43e2 Feb 5, 2016

highfive added the S-needs-rebase label Feb 7, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 18, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 18, 2016

highfive added the S-tests-failed label Feb 18, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 18, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 18, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Feb 18, 2016

bors-servo merged commit 066c5ea into servo:master Feb 18, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Feb 18, 2016

Implement XHR::SetRequestHeader Step 3 #9598

Merged

Bypass GetResponseXML to better match spec #9610

Merged

disallow restricted XMLHttpRequest header prefixes #9376

Closed

KiChjang deleted the KiChjang:xhr-cleanup branch Oct 19, 2017

servo / servo

Clean up XHR API #9383

Clean up XHR API #9383

KiChjang commented Jan 20, 2016

highfive commented Jan 20, 2016

This comment has been minimized.

This comment has been minimized.

Ms2ger commented Jan 20, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Jan 20, 2016

KiChjang commented Jan 27, 2016

Ms2ger commented Feb 1, 2016

KiChjang commented Feb 1, 2016

Ms2ger commented Feb 4, 2016

KiChjang commented Feb 5, 2016

KiChjang commented Feb 5, 2016

bors-servo commented Feb 7, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

jdm commented Feb 18, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

KiChjang commented Feb 18, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

KiChjang commented Feb 18, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

servo / servo

Join GitHub today

Clean up XHR API #9383

Clean up XHR API #9383

Conversation

KiChjang commented Jan 20, 2016

highfive commented Jan 20, 2016

This comment has been minimized.

frewsxcv Jan 20, 2016

This comment has been minimized.

frewsxcv Jan 20, 2016

Ms2ger commented Jan 20, 2016

This comment has been minimized.

jdm Jan 20, 2016

This comment has been minimized.

KiChjang Jan 20, 2016

This comment has been minimized.

jdm Jan 20, 2016

KiChjang commented Jan 20, 2016

KiChjang commented Jan 27, 2016

Ms2ger commented Feb 1, 2016

KiChjang commented Feb 1, 2016

Ms2ger commented Feb 4, 2016

KiChjang commented Feb 5, 2016

KiChjang commented Feb 5, 2016

bors-servo commented Feb 7, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

jdm commented Feb 18, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

KiChjang commented Feb 18, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016

KiChjang commented Feb 18, 2016

bors-servo commented Feb 18, 2016

bors-servo commented Feb 18, 2016