Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement XMLHttpRequest.send(Document) #10604
Conversation
highfive
commented
Apr 14, 2016
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @wafflespeanut (or someone else) soon. |
highfive
commented
Apr 14, 2016
|
Heads up! This PR modifies the following files:
|
|
-S-awaiting-review +S-needs-code-changes Reviewed 5 of 5 files at r1. components/script/dom/document.rs, line 1823 [r1] (raw file): components/script/dom/xmlhttprequest.rs, line 533 [r1] (raw file): components/script/dom/xmlhttprequest.rs, line 1442 [r1] (raw file): components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r1] (raw file): Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. components/script/dom/xmlhttprequest.rs, line 533 [r1] (raw file): components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r1] (raw file): Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed. components/script/dom/document.rs, line 1823 [r1] (raw file): if let Ok(()) = serialize(&mut writer, &self.upcast::<Node>(),
SerializeOpts {traversal_scope: ChildrenOnly,..Default::default()}) {
Ok(DOMString::from(String::from_utf8(writer).unwrap()))
} else {
Err(Error::InvalidState)
}components/script/dom/xmlhttprequest.rs, line 1442 [r1] (raw file): components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed. components/script/dom/document.rs, line 1823 [r1] (raw file): components/script/dom/xmlhttprequest.rs, line 1442 [r1] (raw file): components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r1] (raw file): Comments from Reviewable |
|
Review status: 3 of 8 files reviewed at latest revision, 2 unresolved discussions. components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r1] (raw file): Comments from Reviewable |
|
-S-awaiting-review +S-needs-code-changes +S-blocked-on-external Reviewed 5 of 5 files at r2. components/script/dom/xmlhttprequest.rs, line 1447 [r2] (raw file): components/script/dom/webidls/XMLHttpRequest.webidl, line 58 [r1] (raw file): tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r1] (raw file): Comments from Reviewable |
KiChjang
added
S-blocked-on-external
S-needs-code-changes
and removed
S-awaiting-review
labels
|
Review status: all files reviewed at latest revision, 1 unresolved discussion. components/script/dom/xmlhttprequest.rs, line 1447 [r2] (raw file): Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 1 unresolved discussion. components/script/dom/xmlhttprequest.rs, line 1447 [r2] (raw file): Comments from Reviewable |
|
Review status: 5 of 7 files reviewed at latest revision, 1 unresolved discussion. components/script/dom/xmlhttprequest.rs, line 1447 [r2] (raw file): Comments from Reviewable |
|
Reviewed 3 of 3 files at r3. tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r3] (raw file): Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 1 unresolved discussion. tests/wpt/web-platform-tests/XMLHttpRequest/send-entity-body-document.htm, line 21 [r3] (raw file): Comments from Reviewable |
|
I'm not sure why it worked, but I'm glad it did :) |
|
Hey guys, I've pushed just the code moving because I want to check 2 tests with you. When the shift-js runs it returns some mojibake and fail. I did this, was it right? |
| fn serialize_document(document: &Document) -> Fallible<DOMString> { | ||
| let mut writer = vec![]; | ||
| if let Ok(()) = serialize(&mut writer, &document.upcast::<Node>(), | ||
| SerializeOpts { traversal_scope: ChildrenOnly, ..Default::default() }) { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Sep 20, 2016
Member
IIRC traversal_scope is ChildrenOnly by default, so the second argument can just be Default::default() instead.
| // Serialize a Document struct | ||
| fn serialize_document(document: &Document) -> Fallible<DOMString> { | ||
| let mut writer = vec![]; | ||
| if let Ok(()) = serialize(&mut writer, &document.upcast::<Node>(), |
This comment has been minimized.
This comment has been minimized.
KiChjang
Sep 20, 2016
Member
if serialize(&mut writer, &document.upcast::<Node>(), Default::default()).is_ok() {
| let boundary = generate_boundary(); | ||
| let bytes = encode_multipart_form_data(&mut formdata.datums(), boundary.clone(), | ||
| UTF_8 as EncodingRef); | ||
| (bytes, Some(DOMString::from(format!("multipart/form-data;boundary={}", boundary)))) | ||
| }, | ||
| DocumentOrBodyInit::Document(ref d) => { |
This comment has been minimized.
This comment has been minimized.
KiChjang
Sep 20, 2016
•
Member
A document is not Extractable. This should be done in fn Send(), under step 4 (first half).
|
|
|
@jaysonsantos Is this still being worked on? |
|
@KiChjang Hi, I did a small pause because my newborn is here, my local branch is already rebased but I didn't push to avoid you guys doing unnecessary code review. If you think this is critical you can assign to a new person, otherwise, I will get back to it probably in a few days. |
|
Let us know when you want to work on this again, in the meantime, I'm closing this.Thanks for your work so far! |
|
@KiChjang just a check even though this pr is not open anymore. I moved the serialize_document outside of the Which shouldn't happen because on the webidl it has an alias |
|
|
|
@KiChjang I think I got it. But if I implement |
|
So, 4 possible solutions:
4 sounds like the easiest, since you can just use a match expression and wrap let body = match data {
// ...
DocumentOrBodyInit::String(s) => {
let body_init = BodyInit::String(s);
body_init.extract()
}
// ...
};... though it may sound a bit silly, since you're creating another wrapper struct just to call a method on the wrapper. It may just be better to go with 2, since you'd be able to do the following: let body = match data {
// ...
DocumentOrBodyInit::String(s) => s.extract(),
// ...
};1 is not acceptable, since it deviates from the spec, and aside from the difficulty of patching CodegenRust.py, I have reservations on 3 as to whether that's the best way to move forward. |
|
Hi @KiChjang I came up with this diff --git a/components/script/dom/xmlhttprequest.rs b/components/script/dom/xmlhttprequest.rs
index 462ba85..3c215fd 100644
--- a/components/script/dom/xmlhttprequest.rs
+++ b/components/script/dom/xmlhttprequest.rs
@@ -8,7 +8,7 @@ use dom::bindings::codegen::Bindings::BlobBinding::BlobMethods;
use dom::bindings::codegen::Bindings::DocumentBinding::DocumentMethods;
use dom::bindings::codegen::Bindings::EventHandlerBinding::EventHandlerNonNull;
use dom::bindings::codegen::Bindings::WindowBinding::WindowMethods;
-use dom::bindings::codegen::Bindings::XMLHttpRequestBinding;
+use dom::bindings::codegen::Bindings::XMLHttpRequestBinding::{BodyInit, self};
use dom::bindings::codegen::Bindings::XMLHttpRequestBinding::XMLHttpRequestMethods;
use dom::bindings::codegen::Bindings::XMLHttpRequestBinding::XMLHttpRequestResponseType;
use dom::bindings::codegen::UnionTypes::DocumentOrBodyInit;
@@ -25,6 +25,7 @@ use dom::document::{Document, IsHTMLDocument};
use dom::document::DocumentSource;
use dom::event::{Event, EventBubbles, EventCancelable};
use dom::eventtarget::EventTarget;
+use dom::formdata::FormData;
use dom::globalscope::GlobalScope;
use dom::headers::is_forbidden_header_name;
use dom::htmlformelement::{encode_multipart_form_data, generate_boundary};
@@ -32,6 +33,7 @@ use dom::node::Node;
use dom::progressevent::ProgressEvent;
use dom::servoparser::html::{ParseContext, parse_html};
use dom::servoparser::xml::{self, parse_xml};
+use dom::urlsearchparams::URLSearchParams;
use dom::window::Window;
use dom::workerglobalscope::WorkerGlobalScope;
use dom::xmlhttprequesteventtarget::XMLHttpRequestEventTarget;
@@ -519,20 +521,45 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
_ => data
};
// Step 4 (first half)
- let extracted = data.as_ref().map(|d| d.extract());
+ let extracted = match data {
+ Some(DocumentOrBodyInit::Document(ref d)) => {
+ let data: Vec<u8> = serialize_document(d).unwrap().into();
+ let decoded_data: Vec<u8> = match &*d.CharacterSet() {
+ "UTF-8" => {
+ debug!("Document is already utf-8, skipping conversion {:?}", d.url());
+ data
+ },
+ document_charset => {
+ debug!("Document is {:?} we have to decode", document_charset);
+ let charset = encoding_from_whatwg_label(&*d.CharacterSet()).unwrap_or(UTF_8);
+ charset.decode(&*data, DecoderTrap::Replace).unwrap().into_bytes()
+ }
+ };
+ let mut content_type = String::new();
+ if d.is_html_document() {
+ content_type.push_str("text/html");
+ } else {
+ content_type.push_str("application/xml");
+ };
+ content_type.push_str(";charset=UTF-8");
+ (decoded_data, Some(DOMString::from(content_type)))
+ },
+ Some(DocumentOrBodyInit::Blob(ref blob)) => blob.extract(),
+ Some(DocumentOrBodyInit::FormData(ref form_data)) => form_data.extract(),
+ Some(DocumentOrBodyInit::String(ref s)) => s.extract(),
+ Some(DocumentOrBodyInit::Blob(ref usp)) => usp.extract()
+ };
- self.request_body_len.set(extracted.as_ref().map_or(0, |e| e.0.len()));
+ self.request_body_len.set(extracted.0.len());
// todo preserved headers?
// Step 6
self.upload_complete.set(false);
// Step 7
- self.upload_complete.set(match extracted {
- None => true,
- Some (ref e) if e.0.is_empty() => true,
- _ => false
- });
+ if extracted.0.is_empty() {
+ self.upload_complete.set(true);
+ }
// Step 8
self.send_flag.set(true);
@@ -590,7 +617,7 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
headers: (*self.request_headers.borrow()).clone(),
unsafe_request: true,
// XXXManishearth figure out how to avoid this clone
- body: extracted.as_ref().map(|e| e.0.clone()),
+ body: Some(extracted.0.clone()),
// XXXManishearth actually "subresource", but it doesn't exist
// https://github.com/whatwg/xhr/issues/71
destination: Destination::None,
@@ -612,7 +639,7 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
// step 4 (second half)
match extracted {
- Some((_, ref content_type)) => {
+ (_, ref content_type) => {
// this should handle Document bodies too, not just BodyInit
let encoding = if let Some(DocumentOrBodyInit::String(_)) = data {
// XHR spec differs from http, and says UTF-8 should be in capitals,
@@ -1370,61 +1397,47 @@ pub trait Extractable {
fn extract(&self) -> (Vec<u8>, Option<DOMString>);
}
-impl Extractable for DocumentOrBodyInit {
+impl Extractable for Root<Blob> {
// https://fetch.spec.whatwg.org/#concept-bodyinit-extract
fn extract(&self) -> (Vec<u8>, Option<DOMString>) {
- match *self {
- DocumentOrBodyInit::String(ref s) => {
- let encoding = UTF_8 as EncodingRef;
- (encoding.encode(s, EncoderTrap::Replace).unwrap(),
- Some(DOMString::from("text/plain;charset=UTF-8")))
- }
- DocumentOrBodyInit::URLSearchParams(ref usp) => {
- // Default encoding is UTF-8.
- (usp.serialize(None).into_bytes(),
- Some(DOMString::from("application/x-www-form-urlencoded;charset=UTF-8")))
- }
- DocumentOrBodyInit::Blob(ref b) => {
- let content_type = if b.Type().as_ref().is_empty() {
- None
- } else {
- Some(b.Type())
- };
- let bytes = b.get_bytes().unwrap_or(vec![]);
- (bytes, content_type)
- }
- DocumentOrBodyInit::FormData(ref formdata) => {
- let boundary = generate_boundary();
- let bytes = encode_multipart_form_data(&mut formdata.datums(), boundary.clone(),
- UTF_8 as EncodingRef);
- (bytes, Some(DOMString::from(format!("multipart/form-data;boundary={}", boundary))))
- },
- DocumentOrBodyInit::Document(ref d) => {
- let data: Vec<u8> = serialize_document(d).unwrap().into();
- let decoded_data: Vec<u8> = match &*d.CharacterSet() {
- "UTF-8" => {
- debug!("Document is already utf-8, skipping conversion {:?}", d.url());
- data
- },
- document_charset => {
- debug!("Document is {:?} we have to decode", document_charset);
- let charset = encoding_from_whatwg_label(&*d.CharacterSet()).unwrap_or(UTF_8);
- charset.decode(&*data, DecoderTrap::Replace).unwrap().into_bytes()
- }
- };
- let mut content_type = String::new();
- if d.is_html_document() {
- content_type.push_str("text/html");
- } else {
- content_type.push_str("application/xml");
- };
- content_type.push_str(";charset=UTF-8");
- (decoded_data, Some(DOMString::from(content_type)))
- }
- }
+ let content_type = if self.Type().as_ref().is_empty() {
+ None
+ } else {
+ Some(self.Type())
+ };
+ let bytes = self.get_bytes().unwrap_or(vec![]);
+ (bytes, content_type)
}
}
+impl Extractable for Root<FormData> {
+ // https://fetch.spec.whatwg.org/#concept-bodyinit-extract
+ fn extract(&self) -> (Vec<u8>, Option<DOMString>) {
+ let boundary = generate_boundary();
+ let bytes = encode_multipart_form_data(&mut self.datums(), boundary.clone(),
+ UTF_8 as EncodingRef);
+ (bytes, Some(DOMString::from(format!("multipart/form-data;boundary={}", boundary))))
+ }
+}
+
+impl Extractable for DOMString {
+ // https://fetch.spec.whatwg.org/#concept-bodyinit-extract
+ fn extract(&self) -> (Vec<u8>, Option<DOMString>) {
+ let encoding = UTF_8 as EncodingRef;
+ (encoding.encode(self, EncoderTrap::Replace).unwrap(),
+ Some(DOMString::from("text/plain;charset=UTF-8")))
+ }
+}
+
+impl Extractable for Root<URLSearchParams> {
+ // https://fetch.spec.whatwg.org/#concept-bodyinit-extract
+ fn extract(&self) -> (Vec<u8>, Option<DOMString>) {
+ // Default encoding is UTF-8.
+ (self.serialize(None).into_bytes(),
+ Some(DOMString::from("application/x-www-form-urlencoded;charset=UTF-8")))
+ }
+}
+
/// Returns whether `bs` is a `field-value`, as defined by
/// [RFC 2616](http://tools.ietf.org/html/rfc2616#page-32).
pub fn is_field_value(slice: &[u8]) -> bool {
@@ -1492,8 +1505,8 @@ pub fn is_field_value(slice: &[u8]) -> bool {
// Serialize a Document struct
fn serialize_document(document: &Document) -> Fallible<DOMString> {
let mut writer = vec![];
- if let Ok(()) = serialize(&mut writer, &document.upcast::<Node>(),
- SerializeOpts { traversal_scope: ChildrenOnly, ..Default::default() }) {
+ if serialize(&mut writer, &document.upcast::<Node>(),
+ SerializeOpts { traversal_scope: ChildrenOnly, ..Default::default() }).is_ok() {
Ok(DOMString::from(String::from_utf8(writer).unwrap()))
} else {
Err(Error::InvalidState)but without implementing it on Extractable I will repeat the code a lot because other places use it like here: any suggestions? i thought about creating a function that matches with values to return the tuple from extract but i am afraid of breaking the "code rule" |
|
The easiest way here is to simply implement |
|
Also, instead of implementing traits on |
To do it I would have to make a match on DocumentOrBodyInit and create a BodyInit, right? That |
|
Hey @KiChjang I was trying to implement what you said and I got a really dumb error with the ownership but all the ways I tried to look real ugly and I thought that it might be because the way I tried to do the last changes. When I match data to extract the values, it's ownership is being transferred and it is being used down in the same function, I thought of using match as reference but when I started putting |
|
Yeah, that's a tricky one. You probably need to do something like this: Some(DocumentOrBodyInit::Blob(ref blob)) => {
let blob = BodyInit::Blob(Root::from_ref(&**blob)); |
|
is it too ugly / is it possible to do something like this? let (extracted, data) = match data {
Some(...) => (Some(extracted_data), data),
_ => (None, data)
} |
|
Seems reasonable. |
|
I tried both ways and in the end this happened: When using Root:from_ref i tried both with two derefs and without and the result looks like the same. |
|
Two things - when moving data back out, you want to get rid of the |
jaysonsantos commentedApr 14, 2016
If you guys need a rebase just tell me and I do it.
When serializing document I am using unwrap, should I change
extractedreturn to be a result?Fixes #9490
This change is