Auto merge of #9598 - TimNN:xhr-header-step-3, r=Ms2ger

Implement XHR::SetRequestHeader Step 3

Closes #9548.

Alternative implementation of #9595.

cc @timvandermeij

I'm not sure if a utility method on `ByteString` is the best place for this functionality, maybe a free function in XHR module would be more suitable.

Also where would be the correct place to add a test for this functionality (if required)?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9598)
<!-- Reviewable:end -->

components/script/dom/bindings/str.rs

@@ -48,76 +48,6 @@ impl ByteString {
     pub fn to_lower(&self) -> ByteString {
         ByteString::new(self.0.to_ascii_lowercase())
     }
-
-    /// Returns whether `self` is a `token`, as defined by
-    /// [RFC 2616](http://tools.ietf.org/html/rfc2616#page-17).
-    pub fn is_token(&self) -> bool {
-        is_token(&self.0)
-    }
-
-    /// Returns whether `self` is a `field-value`, as defined by
-    /// [RFC 2616](http://tools.ietf.org/html/rfc2616#page-32).
-    pub fn is_field_value(&self) -> bool {
-        // Classifications of characters necessary for the [CRLF] (SP|HT) rule
-        #[derive(PartialEq)]
-        enum PreviousCharacter {
-            Other,
-            CR,
-            LF,
-            SPHT, // SP or HT
-        }
-        let mut prev = PreviousCharacter::Other; // The previous character
-        self.0.iter().all(|&x| {
-            // http://tools.ietf.org/html/rfc2616#section-2.2
-            match x {
-                13  => { // CR
-                    if prev == PreviousCharacter::Other || prev == PreviousCharacter::SPHT {
-                        prev = PreviousCharacter::CR;
-                        true
-                    } else {
-                        false
-                    }
-                },
-                10 => { // LF
-                    if prev == PreviousCharacter::CR {
-                        prev = PreviousCharacter::LF;
-                        true
-                    } else {
-                        false
-                    }
-                },
-                32 => { // SP
-                    if prev == PreviousCharacter::LF || prev == PreviousCharacter::SPHT {
-                        prev = PreviousCharacter::SPHT;
-                        true
-                    } else if prev == PreviousCharacter::Other {
-                        // Counts as an Other here, since it's not preceded by a CRLF
-                        // SP is not a CTL, so it can be used anywhere
-                        // though if used immediately after a CR the CR is invalid
-                        // We don't change prev since it's already Other
-                        true
-                    } else {
-                        false
-                    }
-                },
-                9 => { // HT
-                    if prev == PreviousCharacter::LF || prev == PreviousCharacter::SPHT {
-                        prev = PreviousCharacter::SPHT;
-                        true
-                    } else {
-                        false
-                    }
-                },
-                0...31 | 127 => false, // CTLs
-                x if x > 127 => false, // non ASCII
-                _ if prev == PreviousCharacter::Other || prev == PreviousCharacter::SPHT => {
-                    prev = PreviousCharacter::Other;
-                    true
-                },
-                _ => false // Previous character was a CR/LF but not part of the [CRLF] (SP|HT) rule
-            }
-        })
-    }
 }
 
 impl Into<Vec<u8>> for ByteString {

components/script/dom/xmlhttprequest.rs

@@ -21,7 +21,7 @@ use dom::bindings::js::{JS, MutHeapJSVal, MutNullableHeap};
 use dom::bindings::js::{Root, RootedReference};
 use dom::bindings::refcounted::Trusted;
 use dom::bindings::reflector::{Reflectable, reflect_dom_object};
-use dom::bindings::str::{ByteString, USVString};
+use dom::bindings::str::{ByteString, USVString, is_token};
 use dom::blob::Blob;
 use dom::document::DocumentSource;
 use dom::document::{Document, IsHTMLDocument};
@@ -55,6 +55,7 @@ use std::ascii::AsciiExt;
 use std::borrow::ToOwned;
 use std::cell::{Cell, RefCell};
 use std::default::Default;
+use std::str;
 use std::sync::{Arc, Mutex};
 use string_cache::Atom;
 use time;
@@ -345,7 +346,7 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
             Some(Method::Extension(ref t)) if &**t == "TRACK" => Err(Error::Security),
             Some(parsed_method) => {
                 // Step 3
-                if !method.is_token() {
+                if !is_token(&method) {
                     return Err(Error::Syntax)
                 }
 
@@ -406,14 +407,17 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
     }
 
     // https://xhr.spec.whatwg.org/#the-setrequestheader()-method
-    fn SetRequestHeader(&self, name: ByteString, mut value: ByteString) -> ErrorResult {
+    fn SetRequestHeader(&self, name: ByteString, value: ByteString) -> ErrorResult {
         // Step 1, 2
         if self.ready_state.get() != XMLHttpRequestState::Opened || self.send_flag.get() {
             return Err(Error::InvalidState);
         }
-        // FIXME(#9548): Step 3. Normalize value
+
+        // Step 3
+        let value = trim_http_whitespace(&value);
+
         // Step 4
-        if !name.is_token() || !value.is_field_value() {
+        if !is_token(&name) || !is_field_value(&value) {
             return Err(Error::Syntax);
         }
         let name_lower = name.to_lower();
@@ -438,24 +442,24 @@ impl XMLHttpRequestMethods for XMLHttpRequest {
             None => unreachable!()
         };
 
-        debug!("SetRequestHeader: name={:?}, value={:?}", name.as_str(), value.as_str());
+        debug!("SetRequestHeader: name={:?}, value={:?}", name.as_str(), str::from_utf8(value).ok());
         let mut headers = self.request_headers.borrow_mut();
 
 
         // Step 6
-        match headers.get_raw(name_str) {
+        let value = match headers.get_raw(name_str) {
             Some(raw) => {
                 debug!("SetRequestHeader: old value = {:?}", raw[0]);
                 let mut buf = raw[0].clone();
                 buf.extend_from_slice(b", ");
-                buf.extend_from_slice(&value);
+                buf.extend_from_slice(value);
                 debug!("SetRequestHeader: new value = {:?}", buf);
-                value = ByteString::new(buf);
+                buf
             },
-            None => {}
+            None => value.into(),
-        }
+        };
 
-        headers.set_raw(name_str.to_owned(), vec![value.into()]);
+        headers.set_raw(name_str.to_owned(), vec![value]);
         Ok(())
     }
 
@@ -1388,3 +1392,91 @@ impl Extractable for SendParam {
         }
     }
 }
+
+/// 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 {
+    // Classifications of characters necessary for the [CRLF] (SP|HT) rule
+    #[derive(PartialEq)]
+    enum PreviousCharacter {
+        Other,
+        CR,
+        LF,
+        SPHT, // SP or HT
+    }
+    let mut prev = PreviousCharacter::Other; // The previous character
+    slice.iter().all(|&x| {
+        // http://tools.ietf.org/html/rfc2616#section-2.2
+        match x {
+            13  => { // CR
+                if prev == PreviousCharacter::Other || prev == PreviousCharacter::SPHT {
+                    prev = PreviousCharacter::CR;
+                    true
+                } else {
+                    false
+                }
+            },
+            10 => { // LF
+                if prev == PreviousCharacter::CR {
+                    prev = PreviousCharacter::LF;
+                    true
+                } else {
+                    false
+                }
+            },
+            32 => { // SP
+                if prev == PreviousCharacter::LF || prev == PreviousCharacter::SPHT {
+                    prev = PreviousCharacter::SPHT;
+                    true
+                } else if prev == PreviousCharacter::Other {
+                    // Counts as an Other here, since it's not preceded by a CRLF
+                    // SP is not a CTL, so it can be used anywhere
+                    // though if used immediately after a CR the CR is invalid
+                    // We don't change prev since it's already Other
+                    true
+                } else {
+                    false
+                }
+            },
+            9 => { // HT
+                if prev == PreviousCharacter::LF || prev == PreviousCharacter::SPHT {
+                    prev = PreviousCharacter::SPHT;
+                    true
+                } else {
+                    false
+                }
+            },
+            0...31 | 127 => false, // CTLs
+            x if x > 127 => false, // non ASCII
+            _ if prev == PreviousCharacter::Other || prev == PreviousCharacter::SPHT => {
+                prev = PreviousCharacter::Other;
+                true
+            },
+            _ => false // Previous character was a CR/LF but not part of the [CRLF] (SP|HT) rule
+        }
+    })
+}
+
+/// Normalize `self`, as defined by
+/// [the Fetch Spec](https://fetch.spec.whatwg.org/#concept-header-value-normalize).
+pub fn trim_http_whitespace(mut slice: &[u8]) -> &[u8] {
+    const HTTP_WS_BYTES: &'static [u8] = b"\x09\x0A\x0D\x20";
+
+    loop {
+        match slice.split_first() {
+            Some((first, remainder)) if HTTP_WS_BYTES.contains(first) =>
+                slice = remainder,
+            _ => break,
+        }
+    }
+
+    loop {
+        match slice.split_last() {
+            Some((last, remainder)) if HTTP_WS_BYTES.contains(last) =>
+                slice = remainder,
+            _ => break,
+        }
+    }
+
+    slice
+}

tests/unit/script/dom/xmlhttprequest.rs

@@ -0,0 +1,27 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use script::dom::xmlhttprequest::trim_http_whitespace;
+
+#[test]
+fn test_trim_http_whitespace() {
+    fn test_trim(in_: &[u8], out: &[u8]) {
+        let b = trim_http_whitespace(in_);
+        assert_eq!(b, out);
+    }
+
+    test_trim(b"", b"");
+
+    test_trim(b" ", b"");
+    test_trim(b"a", b"a");
+    test_trim(b" a", b"a");
+    test_trim(b"a ", b"a");
+    test_trim(b" a ", b"a");
+
+    test_trim(b"\t", b"");
+    test_trim(b"a", b"a");
+    test_trim(b"\ta", b"a");
+    test_trim(b"a\t", b"a");
+    test_trim(b"\ta\t", b"a");
+}

tests/unit/script/lib.rs

@@ -11,4 +11,5 @@ extern crate util;
 #[cfg(test)] mod dom {
     mod bindings;
     mod blob;
+    mod xmlhttprequest;
 }