Implement Blob response for XMLHttpRequest: #9623 #9629

dlrobertson · 2016-02-14T04:25:02Z

My first attempt at implementing the Blob response for XMLHttpRequest. The expected result for the response tests/wpt/web-platform-test/HMLHttpRequest/response-blob-data.htm is also changed to PASS. Please let me know if you see any areas in which I can improve this PR!

Fixes #9623

KiChjang · 2016-02-14T04:27:37Z

components/script/dom/xmlhttprequest.rs

@@ -726,6 +729,32 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
                        return NullValue();
                    }
                }
+                XMLHttpRequestResponseType::Blob => {
+                    match self.response_blob.get() {


This should really go into a separate method down there, named blob_response, with an annotation containing the corresponding link to the WHATWG specification.

dlrobertson · 2016-02-14T04:28:06Z

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.

components/script/dom/xmlhttprequest.rs, line 126 [r1] (raw file):
From some review of other commits, I guessed that this was the correct type.

components/script/dom/xmlhttprequest.rs, line 757 [r1] (raw file):
It would be nice to implement this as a separate function, but I don't have a good grasp on codegen and how the XMLHttpRequest trait is generated yet.

Comments from the review on Reviewable.io

KiChjang · 2016-02-14T04:29:17Z

tests/wpt/metadata/XMLHttpRequest/response-data-blob.htm.ini

@@ -1,5 +1,5 @@
 [response-data-blob.htm]
  type: testharness
  [XMLHttpRequest: The response attribute: Blob data]
-    expected: FAIL
+    expected: PASS


This is not how we change test expectations. If it is expected to PASS, you can just delete the line. In fact, since there's only one expected test failure in this .ini file, you can delete the entire file.

dlrobertson · 2016-02-14T04:29:23Z

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/xmlhttprequest.rs, line 733 [r1] (raw file):
Beat me to it 😄... I'll figure it out and make the update. Thanks!

Comments from the review on Reviewable.io

dlrobertson · 2016-02-14T04:30:03Z

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

tests/wpt/metadata/XMLHttpRequest/response-data-blob.htm.ini, line 4 [r1] (raw file):
Good to know! I'll add this in the update as well!

Comments from the review on Reviewable.io

KiChjang · 2016-02-14T05:09:34Z

components/script/dom/xmlhttprequest.rs

@@ -1038,6 +1052,25 @@ impl XMLHttpRequest {
        encoding.decode(&self.response.borrow(), DecoderTrap::Replace).unwrap().to_owned()
    }

+    fn blob_response(&self) -> Root<Blob> {


Spec link above this function, please. (https://xhr.spec.whatwg.org/#blob-response)

KiChjang · 2016-02-14T05:10:19Z

components/script/dom/xmlhttprequest.rs

-                }
+                },
+                XMLHttpRequestResponseType::Blob => {
+                    // Step 1


This step should also be moved to blob_response. All we should have here is a single call to blob_response.

KiChjang · 2016-02-14T05:40:23Z

components/script/dom/xmlhttprequest.rs

+    fn blob_response(&self) -> Root<Blob> {
+        // Step 1
+        match self.response_blob.get() {
+            Some(response) => response,


We could simplify this step as if let Some(blob) = self.response_blob.get() { return blob; }

KiChjang · 2016-02-14T05:41:43Z

components/script/dom/xmlhttprequest.rs

+            Some(response) => response,
+            None => {
+                // Step 2
+                let mime: String = match self.response_headers.borrow().get() {


There is a final_mime_type() function defined, so this isn't necessary.

KiChjang · 2016-02-14T05:44:02Z

components/script/dom/xmlhttprequest.rs

+
+                // Step 4
+                let blob = Blob::new(self.global().r(), body, &mime);
+                self.response_blob.set(Some(blob.r()));


This wouldn't compile.

Ignore what I said, MutNullableHeap::set requires an Option.

KiChjang · 2016-02-14T05:48:16Z

components/script/dom/xmlhttprequest.rs

+
+                // Step 3
+                let bytes = self.response.borrow();
+                let body = if bytes.is_empty() {


This step here doesn't seem necessary. I think let blob = Blob::new(self.global().r(), self.response.borrow().to_vec(), &mime); down there would be sufficient.

dlrobertson · 2016-02-14T05:55:28Z

Review status: 0 of 2 files reviewed at latest revision, 10 unresolved discussions.

components/script/dom/xmlhttprequest.rs, line 1054 [r3] (raw file):
Awesome, i'll look it up.

components/script/dom/xmlhttprequest.rs, line 1061 [r3] (raw file):
Okay! I was just being on the safe side. I'll test and use this if possible.

Comments from the review on Reviewable.io

KiChjang · 2016-02-14T06:28:41Z

components/script/dom/xmlhttprequest.rs

+            return response;
+        }
+        // Step 2
+        let mime = match self.final_mime_type() {


let mime = self.fial_mime_type().unwrap_or("").to_string();

KiChjang · 2016-02-14T06:29:25Z

Almost there! r=me with that final nit.

KiChjang · 2016-02-14T07:38:52Z

components/script/dom/xmlhttprequest.rs

+            return response;
+        }
+        // Step 2
+        let mime = self.final_mime_type().map(|x| x.to_string()).unwrap_or("".to_owned());


use std::string::ToString

and then .as_ref().map(ToString::to_string) should work.

KiChjang · 2016-02-14T07:44:53Z

Ah, brainfarted. Everything looks fine right now, thanks for working on this!

@bors-servo r+

bors-servo · 2016-02-14T07:44:54Z

📌 Commit 5dc108b has been approved by KiChjang

bors-servo · 2016-02-14T07:44:55Z

⌛ Testing commit 5dc108b with merge a38442d...


        Auto merge of #9629 - danlrobertson:i9623, r=KiChjang

Implement Blob response for XMLHttpRequest: #9623 My first attempt at implementing the Blob response for XMLHttpRequest. The expected result for the response `tests/wpt/web-platform-test/HMLHttpRequest/response-blob-data.htm` is also changed to `PASS`. Please let me know if you see any areas in which I can improve this PR! Fixes #9623  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9629)

bors-servo · 2016-02-14T07:48:23Z

💔 Test failed - mac-dev-unit

bors-servo · 2016-02-14T23:53:50Z

⌛ Testing commit 0b0e147 with merge 6aa908a...


        Auto merge of #9629 - danlrobertson:i9623, r=KiChjang

Implement Blob response for XMLHttpRequest: #9623 My first attempt at implementing the Blob response for XMLHttpRequest. The expected result for the response `tests/wpt/web-platform-test/HMLHttpRequest/response-blob-data.htm` is also changed to `PASS`. Please let me know if you see any areas in which I can improve this PR! Fixes #9623  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9629)

bors-servo · 2016-02-15T00:41:56Z

💔 Test failed - linux-rel

KiChjang · 2016-02-15T00:47:58Z

@bors-servo retry

#9033

bors-servo · 2016-02-15T01:29:34Z

⌛ Testing commit 0b0e147 with merge d8e1237...


        Auto merge of #9629 - danlrobertson:i9623, r=KiChjang

Implement Blob response for XMLHttpRequest: #9623 My first attempt at implementing the Blob response for XMLHttpRequest. The expected result for the response `tests/wpt/web-platform-test/HMLHttpRequest/response-blob-data.htm` is also changed to `PASS`. Please let me know if you see any areas in which I can improve this PR! Fixes #9623  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9629)

bors-servo · 2016-02-15T02:17:26Z

💔 Test failed - linux-rel

KiChjang · 2016-02-15T02:28:36Z

@bors-servo retry

#9012, #9014

bors-servo · 2016-02-15T03:23:28Z

⌛ Testing commit 0b0e147 with merge e8ae7e4...


        Auto merge of #9629 - danlrobertson:i9623, r=KiChjang

Implement Blob response for XMLHttpRequest: #9623 My first attempt at implementing the Blob response for XMLHttpRequest. The expected result for the response `tests/wpt/web-platform-test/HMLHttpRequest/response-blob-data.htm` is also changed to `PASS`. Please let me know if you see any areas in which I can improve this PR! Fixes #9623  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9629)

bors-servo · 2016-02-15T04:04:06Z

💔 Test failed - mac-rel-css

KiChjang · 2016-02-15T04:04:49Z

@bors-servo retry

#7791

bors-servo · 2016-02-15T04:04:50Z

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

bors-servo · 2016-02-15T04:45:33Z

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

highfive added the S-awaiting-review label Feb 14, 2016

KiChjang reviewed Feb 14, 2016
View changes

KiChjang self-assigned this Feb 14, 2016

dlrobertson force-pushed the dlrobertson:i9623 branch from 78b2830 to 7175fac Feb 14, 2016

KiChjang reviewed Feb 14, 2016
View changes

dlrobertson force-pushed the dlrobertson:i9623 branch from 7175fac to 1070e73 Feb 14, 2016

KiChjang reviewed Feb 14, 2016
View changes

dlrobertson force-pushed the dlrobertson:i9623 branch from 1070e73 to 98a27f6 Feb 14, 2016

KiChjang reviewed Feb 14, 2016
View changes

components/script/dom/xmlhttprequest.rs Outdated

return response;

}

// Step 2

let mime = match self.final_mime_type() {

This comment has been minimized.

Sign in to view

Copy link

KiChjang Feb 14, 2016
Member

let mime = self.fial_mime_type().unwrap_or("").to_string();

dlrobertson force-pushed the dlrobertson:i9623 branch from 98a27f6 to 5dc108b Feb 14, 2016

KiChjang reviewed Feb 14, 2016
View changes

highfive removed the S-awaiting-review label Feb 14, 2016

highfive added the S-awaiting-merge label Feb 14, 2016

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

highfive added the S-awaiting-merge label Feb 14, 2016

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

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

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

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

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

bors-servo merged commit 0b0e147 into servo:master Feb 15, 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 15, 2016

Cache XHR JSON responses #9621

Merged

Clean up XHR API #9383

Merged

add file support to form data #9198

Closed

dlrobertson deleted the dlrobertson:i9623 branch Feb 15, 2016

servo / servo

Implement Blob response for XMLHttpRequest: #9623 #9629

Implement Blob response for XMLHttpRequest: #9623 #9629

dlrobertson commented Feb 14, 2016

This comment has been minimized.

dlrobertson commented Feb 14, 2016

This comment has been minimized.

dlrobertson commented Feb 14, 2016

dlrobertson commented Feb 14, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dlrobertson commented Feb 14, 2016

This comment has been minimized.

KiChjang commented Feb 14, 2016

This comment has been minimized.

KiChjang commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 15, 2016

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

bors-servo commented Feb 15, 2016

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

bors-servo commented Feb 15, 2016

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

bors-servo commented Feb 15, 2016

servo / servo

Join GitHub today

Implement Blob response for XMLHttpRequest: #9623 #9629

Implement Blob response for XMLHttpRequest: #9623 #9629

Conversation

dlrobertson commented Feb 14, 2016

This comment has been minimized.

dlrobertson commented Feb 14, 2016

This comment has been minimized.

dlrobertson commented Feb 14, 2016

dlrobertson commented Feb 14, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dlrobertson commented Feb 14, 2016

This comment has been minimized.

KiChjang commented Feb 14, 2016

This comment has been minimized.

KiChjang commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 14, 2016

bors-servo commented Feb 15, 2016

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

bors-servo commented Feb 15, 2016

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

bors-servo commented Feb 15, 2016

KiChjang commented Feb 15, 2016

bors-servo commented Feb 15, 2016

bors-servo commented Feb 15, 2016