Implemented http fetch (partial #4576)

[KiChjang]

This is a work-in-progress for the implementation of HTTP fetch. Currently, it does not compile, as I haven't figured out how to solve the issues regarding borrows and lifetimes.

[hoppipolla-critic-bot]

Critic review: https://critic.hoppipolla.co.uk/r/4785

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

[jdm]

\o/

[jdm]

@KiChjang Would you like feedback on anything specifically?

[Manishearth]

👯

[KiChjang]

Yes - I've been trying to destructure and extract the cache from the Request object, and I've used self.cache.unwrap() and if let Some(ref cache) = self.cache to do so, but both of these attempts tell me that I cannot move out of borrowed content.

[Manishearth]

What's the spec link for this one?

[KiChjang]

This isn't actually from the spec, it's a workaround for the client (refer to the note you left on the line above).

[Manishearth]

oh, I see.

I'll look through this sometime tomorrow I guess (got some changes to review in humpty_dumpty first)

[jdm]

Usually you want self.cache.as_ref().unwrap() for the first, but that should be equivalent to the if let form you gave. What is the error you get from doing that?

[KiChjang]

Sorry, somehow git messed up my changes, and one of my commits are gone. I'll have to rewrite some code and push it before I can answer.

[bors-servo]

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

[jdm]

@KiChjang What feedback are you looking for here? Is this a PR that you would like to merge after review?

[KiChjang]

Yes, since all the build errors are fixed, I'd like to merge this PR after reviews.

[jdm]

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

---

Reviewed files:

• components/net/fetch/request.rs @ r2

---

^{components/net/fetch/request.rs, line 102 [r2] (raw file):}
usize

---

^{components/net/fetch/request.rs, line 108 [r2] (raw file):}
Why is this an Option?

---

^{components/net/fetch/request.rs, line 168 [r2] (raw file):}
_authentication_fetch_flag and _cors_flag are used, so they shouldn't have the _ prefix.

---

^{components/net/fetch/request.rs, line 186 [r2] (raw file):}
Let's call this method_mismatch.

---

^{components/net/fetch/request.rs, line 187 [r2] (raw file):}
Let's call this header_mismatch.

---

^{components/net/fetch/request.rs, line 207 [r2] (raw file):}
nit: no need for ()

---

^{components/net/fetch/request.rs, line 209 [r2] (raw file):}
This if let seems a bit silly - let's just do:

let preflight_result = self.preflight_fetch();
if preflight_result.response_type == ResponseType::Error {
    return Response::network_error();
}

---

^{components/net/fetch/request.rs, line 225 [r2] (raw file):}
&& not ||

---

^{components/net/fetch/request.rs, line 231 [r2] (raw file):}

let fetch_result = self.http_network_or_cache_fetch(credentials, _authentication_fetch_flag);
if _cors_flag && self.cors_check(&fetch_result).is_err() {
    return Response::network_error();
}
response = Some(fetch_result);

---

^{components/net/fetch/request.rs, line 238 [r2] (raw file):}
There should always be a response by now, so I would prefer let mut response = response.unwrap(); and unindenting the rest of this code.

---

^{components/net/fetch/request.rs, line 251 [r2] (raw file):}
This is step 2.

---

^{components/net/fetch/request.rs, line 255 [r2] (raw file):}
Step 3 says return response (according to https://fetch.spec.whatwg.org/#concept-header-parse null means the header is not present). I don't think hyper exposes "failure to parse" to us, so that's worth a FIXME.

---

^{components/net/fetch/request.rs, line 263 [r2] (raw file):}

let locationUrl = match locationUrl {
    Ok(locationUrl) => locationUrl,
    Err(_) => return Response::network_error(),
};

---

^{components/net/fetch/request.rs, line 273 [r2] (raw file):}
This is not step 9 or step 10.

---

^{components/net/fetch/request.rs, line 279 [r2] (raw file):}
Link to the rust-url issue here, please.

---

^{components/net/fetch/request.rs, line 283 [r2] (raw file):}
!locationUrl.username().unwrap_or("").is_empty()

---

^{components/net/fetch/request.rs, line 286 [r2] (raw file):}
Substep 3 is missing here.

---

^{components/net/fetch/request.rs, line 287 [r2] (raw file):}
Substep 4

---

^{components/net/fetch/request.rs, line 289 [r2] (raw file):}
Substep 5

---

^{components/net/fetch/request.rs, line 325 [r2] (raw file):}
This isn't specified as part of step 6; where does this come from?

---

^{components/net/fetch/request.rs, line 337 [r2] (raw file):}
No step 7 in the spec.

---

^{components/net/fetch/request.rs, line 358 [r2] (raw file):}
http://doc.servo.org/hyper/header/enum.IfRange.html

---

^{components/net/fetch/request.rs, line 365 [r2] (raw file):}
http://doc.servo.org/hyper/header/struct.AcceptLanguage.html and http://doc.servo.org/hyper/header/struct.ContentLanguage.html

---

Comments from the review on Reviewable.io

[KiChjang]

^{components/net/fetch/request.rs, line 108 [r2] (raw file):}
From what I read, the global object can be 1 of 3 values: Window, WorkerGlobalScope or ServiceWorkerGlobalScope. I used that to encapsulate these 3 cases, but I guess a simple boolean would do since the global object is only checked once here in the whole Fetch spec.

---

^{components/net/fetch/request.rs, line 325 [r2] (raw file):}
Oh crap. There are a couple of instances where I scrolled to nowhere in the spec and I got lost scanning the steps that I needed to partake. This is one of them.

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review -S-needs-rebase +S-needs-code-changes
Nearly there! This is looking really good.

---

Reviewed files:

• components/net/fetch/request.rs @ r3

---

^{components/net/fetch/request.rs, line 255 [r2] (raw file):}
This match still isn't right - according to step 3, when there is no header present we should return the response. Additionally, we shouldn't be comparing against the value "null" - if there is a header present, we should go directly to step 5.

---

^{components/net/fetch/request.rs, line 258 [r3] (raw file):}

if ... {
    return Response::network_error();
}

---

^{components/net/fetch/request.rs, line 263 [r3] (raw file):}
While true, what we really mean is "Hyper does not expose the presence of headers that failed to parse."

---

^{components/net/fetch/request.rs, line 358 [r2] (raw file):}
This comment can be removed now.

---

^{components/net/fetch/request.rs, line 365 [r2] (raw file):}
This comment can be removed now.

---

^{components/net/fetch/request.rs, line 375 [r3] (raw file):}
Let's structure this method as:

if h.is::<ContentType>() {
    ...
} else {
    h.is::<Accept>() || h.is::<AcceptLanguage>() || h.is::<ContentLanguage>()
}

---

Comments from the review on Reviewable.io

[KiChjang]

@seanmonstar

[seanmonstar]

Review status: 0 of 1 files reviewed, 4 unresolved discussions, all commit checks successful.

---

^{components/net/fetch/request.rs, line 146 [r4] (raw file):}
Couldn't find anything at this link...

---

^{components/net/fetch/request.rs, line 148 [r4] (raw file):}
This method always errors?

---

^{components/net/fetch/request.rs, line 267 [r2] (raw file):}
One way to do so is to check headers.has::<Foo>(). If it has it, but .get::<Foo>() returns None, that means the Foo::parse_header method failed.

Changing the signature of get() to return Option<Result<H, Error>> seems gross...

---

Comments from the review on Reviewable.io

[KiChjang]

^{components/net/fetch/request.rs, line 146 [r4] (raw file):}
My bad. Let me fix it up on a new commit

---

^{components/net/fetch/request.rs, line 148 [r4] (raw file):}
This and subsequent functions like http_or_cache_fetch all return network_errors because they're unimplemented yet. I plan to only finish up http_fetch in this PR and work on the others later.

---

Comments from the review on Reviewable.io

[seanmonstar]

Review status: 0 of 1 files reviewed, 4 unresolved discussions, all commit checks successful.

---

^{components/net/fetch/request.rs, line 148 [r4] (raw file):}
I don't know what servo's convetions are with this, so if I'm wrong, ignore this, but I personally would use unimplemented!(), to catch that the those methods shouldn't be called yet. Or perhaps they can be called because it's dependent on the DOM calling things, and you want to just return network_error()...

---

Comments from the review on Reviewable.io

[jdm]

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

• components/net/fetch/request.rs @ r7

---

^{components/net/fetch/request.rs, line 148 [r4] (raw file):}
A TODO here wouldn't go amiss, I guess.

---

^{components/net/fetch/request.rs, line 267 [r7] (raw file):}
I don't think we need any special handling here - let's just replace this match with

if location.is_none() {
    return Response::network_error();
}

---

Comments from the review on Reviewable.io

[jdm]

-S-awaiting-review +S-needs-squash

---

Review status: all files reviewed, 2 unresolved discussions, all commit checks successful.
Reviewed files:

• components/net/fetch/request.rs @ r8

---

Comments from the review on Reviewable.io

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 450931a has been approved by jdm

[bors-servo]

⌛ Testing commit 450931a with merge 90aacf0...

[bors-servo]

💔 Test failed - linux1

[jdm]

@bors-servo: retry

• #5928

[bors-servo]

⚡ Previous build results for android, gonk, linux2, mac1, mac2 are reusable. Rebuilding only linux1...

[bors-servo]

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2