Implement the Request API

[jeenalee]

This PR implements the Request API for the Fetch API, including its attributes and constructor, and introduces changes in relevant files.

This Request integrates net_traits::request::Request and dom::headers.

There are few related TODOs and comments:

1. net_traits::request::Request's headers field does not point to dom::request::Request's headers_reflector.
2. Every Constructor step that involves Readable Stream object is not implemented.
3. Every Constructor step that involves entry settings object or environment settings object is not implemented.
4. ./mach build -d does not report any error, but prints a few warnings about unused variables related to (1) and (2).
5. Enum ReferrerPolicy generated by RequestBinding does not match net_traits::request::Request's implementation.
6. Promises in Body webidl are commented out.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #11895 (github issue number if applicable).

• There are tests for these changes OR
• These changes do not require tests because tests for the Request API already exists, but this commit does not implement the interface fully.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/request.rs, components/script/dom/webidls/Request.webidl, components/script/dom/webidls/Body.webidl, components/script/dom/mod.rs, components/script/dom/headers.rs, components/script/dom/bindings/trace.rs

[highfive]

Warning

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

[jeenalee]

r? @jdm

[KiChjang]

This method (Append) here and Delete, Set could really use a match expression instead of using so many ifs. Something like the following:

match *self.guard.borrow() {
    Guard::Immutable => Err(Error::Type("Guard is immutable".to_owned())),
    Guard::Request if is_forbidden_header_name(&valid_name) => Ok(()),
    // ... and so on...
}

[jdm]

I disagree here - I find it easier to follow the logic and match it against the specification when it's split up the way it is.

[KiChjang]

*self.guard.borrow_mut() = new_guard;

[KiChjang]

Not sure what the purpose is in getting a guard here, but a reference to self.guard.borrow() should be returned instead. Cloning here means that the Guard returned is a completely different object than the Guard in self.guard. Also, return can be omitted here.

[KiChjang]

*self.header_list.borrow_mut() = HyperHeaders::new();

[KiChjang]

I believe you can do use net_traits::request::{Request as NetTraitsRequest, Origin, Window};. Can also add in any other struct/enum that you need from net_traits/request.rs

[KiChjang]

use msg::constellation_msg::PipelineId above and you can simply use PipelineId here.

[KiChjang]

use url::Url;

[jdm]

I know I've left a lot of comments, but this is really great work! Most of my comments are about idiomatic usage of RefCell/Cell and reducing unnecessary cloning.
-S-awaiting-review +S-needs-code-changes

---

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 2 files at r4, 3 of 4 files at r5, 1 of 1 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, 1 of 2 files at r13, 1 of 2 files at r14, 1 of 1 files at r15.
Review status: all files reviewed at latest revision, 80 unresolved discussions.

---

components/script/dom/headers.rs, line 21 [r10] (raw file):

pub struct Headers {
    reflector_: Reflector,
    guard: DOMRefCell<Guard>,

This can be Cell instead.

---

components/script/dom/headers.rs, line 27 [r14] (raw file):

// https://fetch.spec.whatwg.org/#concept-headers-guard
#[derive(Clone, JSTraceable, HeapSizeOf, PartialEq)]

This can include Copy as well.

---

components/script/dom/headers.rs, line 210 [r14] (raw file):

    // https://fetch.spec.whatwg.org/#concept-header-extract-mime-type
    pub fn extract_mime_type(&self) -> Vec<u8> {
        let content_type = ByteString::new("Content-Type".to_string().into_bytes());

Let's do self.header_list.borrow().get_raw("content-type").map_or(vec![], |v| v[0].clone()) instead.

---

components/script/dom/request.rs, line 33 [r5] (raw file):

pub struct Request {
    reflector_: Reflector,
    #[ignore_heap_size_of = "net_traits is missing HeapSizeOf implementation"]

We should add one, in that case!

---

components/script/dom/request.rs, line 35 [r5] (raw file):

    #[ignore_heap_size_of = "net_traits is missing HeapSizeOf implementation"]
    request: DOMRefCell<net_traits::request::Request>,
    body_used: DOMRefCell<bool>,

This can be a Cell<bool> instead, since the value can be copied.

---

components/script/dom/request.rs, line 36 [r5] (raw file):

    request: DOMRefCell<net_traits::request::Request>,
    body_used: DOMRefCell<bool>,
    headers_reflector: MutNullableHeap<JS<Headers>>,

Let's call this headers.

---

components/script/dom/request.rs, line 60 [r5] (raw file):

    }

    pub fn new_inherited(url: url::Url,

No need for this to be public.

---

components/script/dom/request.rs, line 94 [r5] (raw file):

        let mut new_request = Request::new(global,
                                           url::Url::parse("").unwrap(),

I have filed whatwg/fetch#351 to sort out what we should actually be doing here.

---

components/script/dom/request.rs, line 61 [r15] (raw file):

    pub fn new(global: GlobalRef,
               url: url::Url,
               origin: Option<NetTraitsRequest::Origin>,

We should obtain an origin based on the URL of the GlobalRef instead of accepting it as an argument here.

---

components/script/dom/request.rs, line 63 [r15] (raw file):

               origin: Option<NetTraitsRequest::Origin>,
               is_service_worker_global_scope: bool,
               pipeline_id: Option<msg::constellation_msg::PipelineId>) -> Root<Request> {

We should use global.pipeline() instead of accepting an argument here.

---

components/script/dom/request.rs, line 93 [r15] (raw file):

                                           None,
                                           false,
                                           None);

We should share this code with the same code in new_inherited.

---

components/script/dom/request.rs, line 103 [r15] (raw file):

        // Step 4
        let window = Cell::new(NetTraitsRequest::Window::Client);

No need for Cell outside of data structures. This can be a regular let mut.

---

components/script/dom/request.rs, line 121 [r15] (raw file):

        // Step 8
        if let Some(url) = get_current_url(&temporary_request) {
            request.url_list = RefCell::new(vec![url.clone()]);

*request.url_list.borrow_mut() = vec![url.clone()];

---

components/script/dom/request.rs, line 129 [r15] (raw file):

        // TODO: `client` is not implemented in Servo yet.
        // ... new_request's client = entry settings object
        request.origin = RefCell::new(NetTraitsRequest::Origin::Client);

*request.origin.borrow_mut() = Origin::Client;

---

components/script/dom/request.rs, line 131 [r15] (raw file):

        request.origin = RefCell::new(NetTraitsRequest::Origin::Client);
        request.omit_origin_header = temporary_request.omit_origin_header;
        request.same_origin_data = Cell::new(true);

request.same_origin_data.set(true);

---

components/script/dom/request.rs, line 142 [r15] (raw file):

        // Step 9
        let mut fallback_mode: Option<NetTraitsRequest::RequestMode>;
        fallback_mode = None;

These two lines can be combined.

---

components/script/dom/request.rs, line 146 [r15] (raw file):

        // Step 10
        let mut fallback_credentials: Option<NetTraitsRequest::CredentialsMode>;
        fallback_credentials = None;

These two lines can be combined.

---

components/script/dom/request.rs, line 150 [r15] (raw file):

        // Step 11
        // TODO: `entry settings object` is not implemented in Servo yet.
        // ... let base_url = entry settings object's API base URL

We can approximate this for now by using global.get_url(). Keep the TODO, however.

---

components/script/dom/request.rs, line 159 [r15] (raw file):

            let parsed_url = url::Url::parse(&usv_string);
            // Step 12.2
            if let &Err(_) = &parsed_url {

if parsed_url.is_err() {

---

components/script/dom/request.rs, line 168 [r15] (raw file):

                }
                // Step 12.4
                request.url_list = RefCell::new(vec![url]);

*request.url_list.borrow_mut() = vec![url];

---

components/script/dom/request.rs, line 188 [r15] (raw file):

            !init.window.is_undefined() {
                // Step 13.1
                if let NetTraitsRequest::RequestMode::Navigate

If we can't use ==, we should add derive(PartialEq) to the RequestMode enum so that we can.

---

components/script/dom/request.rs, line 194 [r15] (raw file):

                    }
                // Step 13.2
                request.omit_origin_header = Cell::new(false);

Use .set instead.

---

components/script/dom/request.rs, line 196 [r15] (raw file):

                request.omit_origin_header = Cell::new(false);
                // Step 13.3
                request.referer = RefCell::new(NetTraitsRequest::Referer::Client);

borrow_mut() instead.

---

components/script/dom/request.rs, line 198 [r15] (raw file):

                request.referer = RefCell::new(NetTraitsRequest::Referer::Client);
                // Step 13.4
                request.referrer_policy = Cell::new(None);

.set

---

components/script/dom/request.rs, line 203 [r15] (raw file):

        // Step 14
        if let Some(init_referrer) = init.referrer.as_ref() {
            let parsed_referrer: url::Url;

This seems to be unused.

---

components/script/dom/request.rs, line 205 [r15] (raw file):

            let parsed_referrer: url::Url;
            // Step 14.1
            let referrer: String = init_referrer.0.clone();

We shouldn't need to clone this here; we can borrow it instead.

---

components/script/dom/request.rs, line 232 [r15] (raw file):

                            // Step 14.7
                            request.referer =

borrow_mut

---

components/script/dom/request.rs, line 242 [r15] (raw file):

        if let Some(init_referrerpolicy) = init.referrerPolicy.as_ref() {
            let init_referrer_policy = init_referrerpolicy.clone().into();
            request.referrer_policy = Cell::new(Some(init_referrer_policy));

.set

---

components/script/dom/request.rs, line 247 [r15] (raw file):

        // Step 16
        let mut mode: Option<NetTraitsRequest::RequestMode>;
        mode = None;

We can write this as let mode = init.mode.as_ref().map(|m| m.clone().into()).or(fallback_mode);.

---

components/script/dom/request.rs, line 259 [r15] (raw file):

        // Step 18
        if mode.is_some() {

Let's use if let to avoid the unwrap.

---

components/script/dom/request.rs, line 265 [r15] (raw file):

        // Step 19
        let credentials: Option<NetTraitsRequest::CredentialsMode>;
        match init.credentials.as_ref() {

Same transformation is possible as mode.

---

components/script/dom/request.rs, line 282 [r15] (raw file):

        // Step 22
        if let NetTraitsRequest::CacheMode::OnlyIfCached = request.cache_mode.get() {

== instead of if let.

---

components/script/dom/request.rs, line 284 [r15] (raw file):

        if let NetTraitsRequest::CacheMode::OnlyIfCached = request.cache_mode.get() {
            match request.mode {
                NetTraitsRequest::RequestMode::SameOrigin => {},

if request.mode != RequestMode::SameOrigin

---

components/script/dom/request.rs, line 298 [r15] (raw file):

        if let Some(init_integrity) = init.integrity.as_ref() {
            let integrity = init_integrity.clone().to_string();
            request.integrity_metadata = RefCell::new(integrity);

.borrow_mut

---

components/script/dom/request.rs, line 303 [r15] (raw file):

        // Step 25
        if let Some(init_method) = init.method.as_ref() {
            let method: ByteString = init_method.clone();

No need to clone this value until necessary.

---

components/script/dom/request.rs, line 312 [r15] (raw file):

            }
            // Step 25.2
            let method_string = method.to_lower().as_str().unwrap().to_string();

The unwrap here means we could panic if we get a sequence of characters that is not valid UTF8. Let's return a type error instead. Also, since we're passing the result to normalize_method (which accepts &str), we don't need to call to_string() here.

---

components/script/dom/request.rs, line 316 [r15] (raw file):

            // Step 25.3
            let hyper_method = from_normalized_method_to_method_enum(&normalized_method);
            request.method = RefCell::new(hyper_method);

borrow_mut

---

components/script/dom/request.rs, line 321 [r15] (raw file):

        // Step 26
        let r = Request::new(global,
                                 url::Url::parse("").unwrap(),

Let's make a Request constructor method that accepts a NetTraitsRequest argument and whatever else we want to initialize instead.

---

components/script/dom/request.rs, line 331 [r15] (raw file):

        *r.request.borrow_mut() = request;
        r.headers_reflector.or_init(|| Headers::new(r.global().r(), None).unwrap());
        r.headers_reflector.get().unwrap().set_guard(Guard::Request);

Let's add a Headers::for_request constructor instead of set_guard.

---

components/script/dom/request.rs, line 334 [r15] (raw file):

        // Step 27
        let headers = r.headers_reflector.get().clone();

r.Headers() will return a non-option value.

---

components/script/dom/request.rs, line 337 [r15] (raw file):

        // Step 28
        let mut headers_init: Option<HeadersOrByteStringSequenceSequence>;

let headers_init = init.headers.as_ref().map(|h| h.clone());

---

components/script/dom/request.rs, line 345 [r15] (raw file):

        // Step 29
        r.headers_reflector.get().unwrap().empty_header_list();

headers.empty_header_list();

---

components/script/dom/request.rs, line 348 [r15] (raw file):

        // Step 30
        if let NetTraitsRequest::RequestMode::NoCORS = r.request.borrow().mode {

== instead of if let

---

components/script/dom/request.rs, line 350 [r15] (raw file):

        if let NetTraitsRequest::RequestMode::NoCORS = r.request.borrow().mode {
            let method = r.request.borrow().method.clone();
            if !is_cors_safelisted_method(method.into_inner()) {

I think this can be if !is_cors_safelisted_method(*r.request.borrow().method.borrow()) instead.

---

components/script/dom/request.rs, line 355 [r15] (raw file):

            }
            let integrity_metadata = r.request.borrow().integrity_metadata.clone();
            if !integrity_metadata.into_inner().is_empty() {

if !r.request.borrow().integrity_metadata.borrow().is_empty() {

---

components/script/dom/request.rs, line 358 [r15] (raw file):

                return Err(Error::Type("Integrity metadata is not an empty string".to_string()));
            }
            r.headers_reflector.get().unwrap().set_guard(Guard::RequestNoCors);

headers.set_guard

---

components/script/dom/request.rs, line 363 [r15] (raw file):

        // Step 31
        if headers_init.is_some() {
            r.headers_reflector.set(Some(Headers::new(global, headers_init).unwrap().r()));

This overwrites the original, which would undo the guard modification we made previously. We should add a method to Headers that accepts another Headers object and implements the fill algorithm.

---

components/script/dom/request.rs, line 367 [r15] (raw file):

        // Step 32
        let mut input_body: Option<Vec<u8>>;

let input_body = if let RequestInfo::Request(input_request) = input {
    input_request.request.borrow().body.clone()
} else {
    None
};

---

components/script/dom/request.rs, line 377 [r15] (raw file):

        // Step 33
        if let Some(init_body_option) = init.body.as_ref() {

if init.body.is_some() || input_body.is_some() {

---

components/script/dom/request.rs, line 403 [r15] (raw file):

        // Step 34
        if let Some(init_body_option) = init.body.as_ref() {
            if let Some(_) = init_body_option.as_ref() {

if init.body.is_some() {

---

components/script/dom/request.rs, line 406 [r15] (raw file):

                // Step 34.1
                let content_type: Option<Vec<u8>>;
                content_type = None;

Let's remove this until it's needed.

---

components/script/dom/request.rs, line 416 [r15] (raw file):

        // Step 35
        r.request.borrow_mut().body = RefCell::new(input_body);

*r.request.borrow().body.borrow_mut() = input_body;

---

components/script/dom/request.rs, line 419 [r15] (raw file):

        // Step 36
        let extracted_mime_type = r.headers_reflector.get().unwrap().extract_mime_type();

headers.extract_mime_type()

---

components/script/dom/request.rs, line 440 [r15] (raw file):

}

fn from_normalized_method_to_method_enum(m: &str) -> hyper::method::Method {

Let's call this normalized_method_to_typed_method

---

components/script/dom/request.rs, line 524 [r15] (raw file):

// https://fetch.spec.whatwg.org/#concept-body-disturbed
fn requestinfo_is_disturbed(input: &RequestInfo) -> bool {
    false

This should delegate to request_is_disturbed.

---

components/script/dom/request.rs, line 530 [r15] (raw file):

// https://fetch.spec.whatwg.org/#concept-body-locked
fn requestinfo_is_locked(input: &RequestInfo) -> bool {
    false

This should delegate to request_is_locked.

---

components/script/dom/request.rs, line 538 [r15] (raw file):

        let r = self.request.borrow().clone();
        let m = r.method.into_inner();
        ByteString::new(Vec::from(m.as_ref().as_bytes()))

let r = self.request.borrow();
let m = r.method.borrow();
ByteString::new(m.as_ref().as_bytes().into());

---

components/script/dom/request.rs, line 544 [r15] (raw file):

    fn Url(&self) -> USVString {
        let r = self.request.borrow().clone();
        let url = r.url_list.into_inner()[0].clone();

let r = self.request.borrow();
USVString(r.url_list.borrow().get(0).map_or("", |u| u.as_str()).into())

---

components/script/dom/request.rs, line 550 [r15] (raw file):

    // https://fetch.spec.whatwg.org/#dom-request-headers
    fn Headers(&self) -> Root<Headers> {
        self.headers_reflector.or_init(|| Headers::new(self.global().r(), None).unwrap())

We should make a constructor method for Headers that has no initializer argument and is infallible. This will let us remove the unwrap.

---

components/script/dom/request.rs, line 556 [r15] (raw file):

    fn Type(&self) -> RequestType {
        let r = self.request.borrow().clone();
        r.type_.into()

self.request.borrow().type_

---

components/script/dom/request.rs, line 562 [r15] (raw file):

    fn Destination(&self) -> RequestDestination {
        let r = self.request.borrow().clone();
        r.destination.into()

self.request.borrow().destination

---

components/script/dom/request.rs, line 569 [r15] (raw file):

        let r = self.request.borrow().clone();
        let referrer = r.referer.into_inner();
        match referrer {

USVString(match *self.request.borrow().referer.borrow() {
    Referer::NoReferer => String::from("no-referrer"),
    ...
})

---

components/script/dom/request.rs, line 580 [r15] (raw file):

        let r = self.request.borrow().clone();
        let rp = r.referrer_policy.get();
        match rp {

self.request.borrow().referrer_policy.get().unwrap_or(ReferrerPolicy::_empty)

---

components/script/dom/request.rs, line 589 [r15] (raw file):

    fn Mode(&self) -> RequestMode {
        let r = self.request.borrow().clone();
        r.mode.into()

self.request.borrow().mode

---

components/script/dom/request.rs, line 614 [r15] (raw file):

        let r = self.request.borrow().clone();
        let integrity = r.integrity_metadata.into_inner();
        DOMString::from_string(integrity)

DOMString::from_string(self.request.borrow().integrity_metadata.borrow().clone())

---

components/script/dom/request.rs, line 625 [r15] (raw file):

    fn Clone(&self) -> Fallible<Root<Request>> {
        // Step 1
        if request_is_locked(&self) {

No need for the &.

---

components/script/dom/request.rs, line 633 [r15] (raw file):

        // Step 2
        let url = self.request.borrow().clone().url_list.into_inner()[0].clone();

let url = self.request.borrow().url();

---

components/script/dom/request.rs, line 639 [r15] (raw file):

        let body_used = self.body_used.borrow().clone();
        let mime_type = self.mime_type.borrow().clone();
        let headers_guard = self.headers_reflector.get().unwrap().get_guard();

This should use Headers() instead, or there may not be a header object.

---

components/script/dom/request.rs, line 640 [r15] (raw file):

        let mime_type = self.mime_type.borrow().clone();
        let headers_guard = self.headers_reflector.get().unwrap().get_guard();
        let r = Request::new(self.global().r(),

Let's add a clone_from constructor that is in charge of extracting the data it needs from the source Request. That will keep the code for constructing new Request objects together in the file.

---

components/script/dom/request.rs, line 647 [r15] (raw file):

        *r.mime_type.borrow_mut() = mime_type;
        *r.body_used.borrow_mut() = body_used;
        r.headers_reflector.get().unwrap().set_guard(headers_guard);

This should use Headers(), since there will not be a headers object.

---

components/script/dom/request.rs, line 652 [r15] (raw file):

}

impl Into<NetTraitsRequest::CacheMode> for RequestCache {

Whoops, when suggesting a bunch of changes earlier I missed that we need to translate between enum types as well. Be sure to add into() to my suggestions as appropriate.

---

components/script/dom/request.rs, line 797 [r15] (raw file):

// ... RequestBinding::ReferrerPolicy does not match msg::constellation_msg::ReferrerPolicy
// ... RequestBinding::ReferrerPolicy has _empty
// ... ... that is not in msg::constellation_msg::ReferrerPolicy

That's because https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer has a step that deals with "the empty string" case.

---

components/script/dom/request.rs, line 799 [r15] (raw file):

// ... ... that is not in msg::constellation_msg::ReferrerPolicy
// ... msg::constellation_msg::ReferrerPolicy has SameOrigin
// ... ... that is not in RequestBinding::ReferrerPolicy

See whatwg/fetch#342 and whatwg/fetch#346

---

Comments from Reviewable

[jeenalee]

components/script/dom/headers.rs, line 21 [r10] (raw file):

Previously, jdm (Josh Matthews) wrote…

This can be Cell instead.

When `Cell` is used, the compiler throws an error about `JSTraceable` and `HeapSizeOf`. What else does this line need if we want to use `Cell`?

---

Comments from Reviewable

[jdm]

Derive JSTraceable and HeapSizeOf for the Guard enum.

[jdm]

@bors-servo: try

[bors-servo]

⌛ Trying commit 65a7651 with merge f5dd27f...

[bors-servo]

💔 Test failed - linux-rel

[jeenalee]

components/script/dom/request.rs, line 614 [r15] (raw file):

Previously, jdm (Josh Matthews) wrote…

DOMString::from_string(self.request.borrow().integrity_metadata.borrow().clone())

This throws a borrow error (https://pastebin.mozilla.org/8890603).

I changed it to:

fn Integrity(&self) -> DOMString {
    let r = self.request.borrow().clone();
    DOMString::from_string(r.integrity_metadata.into_inner())
    }

It still clones self.request. Without clone(), it throws a similar error as the above link.

---

Comments from Reviewable

[jeenalee]

components/script/dom/request.rs, line 544 [r15] (raw file):

let r = self.request.borrow();
USVString(r.url_list.borrow().get(0).map_or("", |u| u.as_str()).into())
I changed it to:

let r = self.request.borrow().clone();
USVString(r.url_list.into_inner().get(0).map_or("", |u| u.as_str()).into())

Without clone(), it throws a borrow error (https://pastebin.mozilla.org/8890609).

---

Comments from Reviewable

[jdm]

Looks like lots of test results need updating, and a couple new failures that need to be addressed:

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's window is not null

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Input URL is not valid

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Input URL has credentials

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's mode is navigate

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's referrer is invalid

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's method is invalid

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's method is forbidden

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's mode is no-cors and method is not simple

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's mode is no-cors and integrity is not empty

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] RequestInit's cache mode is only-if-cached and mode is not same-origin

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Request should get its content-type from the init request

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Request should get its content-type from init headers if one is provided

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Bad referrerPolicy init parameter value

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Bad mode init parameter value

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Bad credentials init parameter value

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Bad cache init parameter value

  ▶ Unexpected subtest result in /fetch/api/request/request-error.html:
  └ PASS [expected FAIL] Bad redirect init parameter value

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid request header "Content-Type: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid request header "Potato: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid request header "proxy: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid request header "proxya: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid request header "sec: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid request header "secb: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Accept-Charset: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "accept-charset: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "ACCEPT-ENCODING: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Accept-Encoding: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Access-Control-Request-Headers: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Access-Control-Request-Method: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Connection: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Content-Length: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Cookie: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Cookie2: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Date: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "DNT: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Expect: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Host: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Keep-Alive: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Origin: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Referer: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "TE: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Trailer: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Transfer-Encoding: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Upgrade: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Via: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Proxy-: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "proxy-a: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "Sec-: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid request header "sec-b: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid no-cors request header "Accept: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid no-cors request header "Accept-Language: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding valid no-cors request header "content-language: OK"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid no-cors request header "Content-Type: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid no-cors request header "Potato: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid no-cors request header "proxy: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid no-cors request header "proxya: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid no-cors request header "sec: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Adding invalid no-cors request header "secb: KO"

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Check that request constructor is filtering headers provided as init parameter

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Check that no-cors request constructor is filtering headers provided as init parameter

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Check that no-cors request constructor is filtering headers provided as part of request parameter

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Request should get its content-type from the init request

  ▶ Unexpected subtest result in /fetch/api/request/request-headers.html:
  └ PASS [expected FAIL] Request should get its content-type from init headers if one is provided

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of GET and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of HEAD and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of POST and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of PUT and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of DELETE and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of OPTIONS and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check method init value of head and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrer init value of /relative/ressource and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrer init value of http://web-platform.test:8000/relative/ressource?query=true#fragment and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrer init value of http://web-platform.test:8000/ and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrerPolicy init value of no-referrer and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrerPolicy init value of no-referrer-when-downgrade and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrerPolicy init value of origin and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrerPolicy init value of origin-when-cross-origin and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check referrerPolicy init value of unsafe-url and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check mode init value of same-origin and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check mode init value of no-cors and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check mode init value of cors and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check credentials init value of omit and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check credentials init value of same-origin and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check credentials init value of include and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check cache init value of default and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check cache init value of no-store and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check cache init value of reload and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check cache init value of no-cache and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check cache init value of force-cache and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check redirect init value of follow and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check redirect init value of error and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check redirect init value of manual and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check integrity init value of  and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check integrity init value of AZERTYUIOP1234567890 and associated getter

  ▶ Unexpected subtest result in /fetch/api/request/request-init-001.sub.html:
  └ PASS [expected FAIL] Check window init value of null and associated getter

  ▶ OK [expected TIMEOUT] /fetch/api/request/request-structure.html

  ▶ Unexpected subtest result in /fetch/api/request/request-structure.html:
  │ FAIL [expected PASS] Check headers attribute
  │   → ({name:"value"}) is not iterable
  │ 
  │ IsreadOnly@http://web-platform.test:8000/fetch/api/request/request-structure.html:42:31
  │ @http://web-platform.test:8000/fetch/api/request/request-structure.html:117:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ test@http://web-platform.test:8000/resources/testharness.js:500:9
  └ @http://web-platform.test:8000/fetch/api/request/request-structure.html:115:9

  ▶ Unexpected subtest result in /fetch/api/request/request-structure.html:
  │ FAIL [expected PASS] Request has arrayBuffer method
  │   → assert_true: request has arrayBuffer method expected true got false
  │ FAIL [expected PASS] Request has blob method
  │   → assert_true: request has blob method expected true got false
  │ FAIL [expected PASS] Request has formData method
  │   → assert_true: request has formData method expected true got false
  │ FAIL [expected PASS] Request has json method
  │   → assert_true: request has json method expected true got false
  │ FAIL [expected PASS] Request has text method
  │   → assert_true: request has text method expected true got false
  │ 
  │ @http://web-platform.test:8000/fetch/api/request/request-structure.html:110:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ test@http://web-platform.test:8000/resources/testharness.js:500:9
  └ @http://web-platform.test:8000/fetch/api/request/request-structure.html:109:9

  ▶ Unexpected subtest result in /fetch/api/request/request-structure.html:
  │ FAIL [expected PASS] Check referrer attribute
  │   → assert_equals: Attribute referrer is read only. Default value is about:client expected "about:client" but got "client"
  │ 
  │ IsreadOnly@http://web-platform.test:8000/fetch/api/request/request-structure.html:104:11
  │ @http://web-platform.test:8000/fetch/api/request/request-structure.html:117:11
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ test@http://web-platform.test:8000/resources/testharness.js:500:9
  └ @http://web-platform.test:8000/fetch/api/request/request-structure.html:115:9

  ▶ Unexpected subtest result in /_mozilla/mozilla/interfaces.html:
  │ FAIL [expected PASS] Interfaces exposed on the window
  │   → assert_true: Body should be defined on the global scope expected true got false
  │ 
  │ test_interfaces/<@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:80:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1402:20
  │ test@http://web-platform.test:8000/resources/testharness.js:500:9
  │ test_interfaces@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:1:3
  └ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.html:6:1

  ▶ Unexpected subtest result in /_mozilla/mozilla/interfaces.worker:
  │ FAIL [expected PASS] Untitled
  │   → assert_true: Body should be defined on the global scope expected true got false
  │ 
  │ test_interfaces/<@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:81:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1403:20
  │ test@http://web-platform.test:8000/resources/testharness.js:501:9
  │ test_interfaces@http://web-platform.test:8000/_mozilla/mozilla/interfaces.js:2:3
  └ @http://web-platform.test:8000/_mozilla/mozilla/interfaces.worker.js:9:1

In particular, Body does not actually define an interface (hence NoInterfaceObject).

[jdm]

Getting close! There are a couple older comments that are still unaddressed, too.
-S-awaiting-review +S-needs-code-changes

---

Reviewed 15 of 15 files at r29.
Review status: all files reviewed at latest revision, 20 unresolved discussions.

---

components/script/dom/headers.rs, line 87 [r29] (raw file):

            let h_list = self.header_list.borrow();
            let previous_value = h_list.get_raw(&valid_name);
            match previous_value {

if let

---

components/script/dom/headers.rs, line 179 [r29] (raw file):

            Some(HeadersOrByteStringSequenceSequence::Headers(h)) => {
                // header_list_copy has type hyper::header::Headers
                let header_list_copy = h.header_list.clone();

The specification was updated to remove the copying here, so header_list_copy can be removed.

---

components/script/dom/headers.rs, line 370 [r29] (raw file):

    if value_len > 2 {
        for &ch in &value[1..value_len - 1] {
            if !is_field_vchar(ch) & !is_space(ch) & !is_htab(ch) {

These should be &&.

---

components/script/dom/request.rs, line 52 [r29] (raw file):

                     origin: Option<Origin>,
                     is_service_worker_global_scope: bool,
                     pipeline_id: Option<PipelineId>) -> Request {

There are unused arguments here, right?

---

components/script/dom/request.rs, line 239 [r29] (raw file):

        // Step 16
        let mut mode = init.mode.as_ref().map(|m| m.clone().into()).or(fallback_mode);

Does this need to be mutable?

---

components/script/dom/request.rs, line 308 [r29] (raw file):

        // Step 26
        let url = request.url();

Where is this used?

---

components/script/dom/request.rs, line 317 [r29] (raw file):

        let mut headers_copy = r.Headers();

        // This is not in the spec.

It would be clearer to phrase this as This is equivalent to the specification's concept of "associated headers list".

---

components/script/dom/request.rs, line 320 [r29] (raw file):

        // Set headers_copy to input's headers.
        if let RequestInfo::Request(ref input_request) = input {
            headers_copy = input_request.headers.get().unwrap().clone();

This could panic if the input request doesn't have any headers. Let's do headers_copy = input_request.Headers() instead.

---

components/script/dom/request.rs, line 354 [r29] (raw file):

        // Step 32
        let mut input_body: Option<Vec<u8>>;
        if let RequestInfo::Request(input_request) = input {

let input_body = if let RequestInfo::Request(input_request) = input {
    let request = input_request.request.borrow();
    request.body.borrow().clone()
else {
    None
};

---

components/script/dom/request.rs, line 402 [r29] (raw file):

                        net_request: NetTraitsRequest) -> Root<Request> {
        let r = Request::new(global,
                             global.get_url(),

This isn't right; we should be taking the URL from the NetTraitsRequest object.

---

Comments from Reviewable

[jdm]

Great work! Please squash these into a single commit except for "Modify Headers API to correctly validate value", which makes sense as an independent one.
-S-awaiting-review +S-needs-squash

---

Reviewed 2 of 2 files at r30.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 69f4cf6 has been approved by jdm

[bors-servo]

⌛ Testing commit 69f4cf6 with merge 78160bf...

[bors-servo]

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev