Refactor http_fetch to reflect the new standard

[KiChjang]

Partial #4576. The spec is really getting funky now - it's depending more and more on the DOM objects (e.g. prompting the user for authentication using a Window object). I think we can just pass in username and password as properties of the Request struct though.

I've also added in the async version for http_request.

[jdm]

I think the most complicated part here is figuring out the best way to avoid cloning the contents of response and actualResponse multiple times, since those could mean significant memory usage for large response bodies.
-S-awaiting-review +S-needs-code-changes

---

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

---

components/net/fetch/request.rs, line 154 [r1] (raw file):
Let's create a method to encapsulate this common concept.

---

components/net/fetch/request.rs, line 215 [r1] (raw file):
This clone shouldn't be necessary.

---

components/net/fetch/request.rs, line 257 [r1] (raw file):
I wonder if this should be Option<&Response> instead, or we should be using Rc<RefCell<Response>> (or maybe just Rc<Response>) to avoid cloning the full response contents at various points in this method. Also, we might be able to use let actual_response; and avoid the unwrap later.

---

components/net/fetch/request.rs, line 369 [r1] (raw file):
I think this reads better as the match.

---

components/net/fetch/request.rs, line 381 [r1] (raw file):
If we write this as else and assert, we should be able to avoid the clone in the previous branch.

---

components/net/fetch/request.rs, line 390 [r1] (raw file):
nit: unnecessary ()

---

components/net_traits/lib.rs, line 107 [r1] (raw file):
vec![]

---

Comments from the review on Reviewable.io

[KiChjang]

Whew! Addressed some issues through 1337 h4x, will look at this later again to figure out what to do with the rest.

---

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

components/net/fetch/request.rs, line 369 [r1] (raw file):
I wanted to, but the match arm will consume the url. I can do Ok(ref url), but I'll have to clone the inner url in order to be assigned to location_url.

---

components/net/fetch/request.rs, line 381 [r1] (raw file):
Might as well use a match statement.

---

Comments from the review on Reviewable.io

[KiChjang]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

components/net/fetch/request.rs, line 369 [r1] (raw file):
Nevermind. Hacking saves the day.

---

Comments from the review on Reviewable.io

[jdm]

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

---

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

---

components/net/fetch/request.rs, line 218 [r2] (raw file):
Can this not be let url = self.url_list.last().unwrap().clone();?

---

components/net/fetch/request.rs, line 370 [r2] (raw file):
Ok(ref url) if url.scheme == "data" => return Response::network_error(),
Ok(url) => url,
_ => return Response::network_error(),

---

Comments from the review on Reviewable.io

[KiChjang]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

components/net/fetch/request.rs, line 257 [r1] (raw file):
I'm beginning to see that the actual_response must at least be an Option since it's nullable. I'm still trying to see whether it's possible to wrap it around a RefCell, a Rc or both together.

---

Comments from the review on Reviewable.io

[KiChjang]

Review status: 1 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

components/net/fetch/request.rs, line 257 [r1] (raw file):
Ok, I think it'll be way easier if I can sort of type-cast one type of pointer into another, e.g. from a Box to a &-pointer, or from an Rc pointer to a Box/&-pointer, etc. Can I actually do that?

---

Comments from the review on Reviewable.io

[jdm]

Smart pointer<T> -> &T is possible with &*.

[KiChjang]

Almost there! I just need to fix a couple of ownership issues.

---

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

---

components/net/fetch/request.rs, line 345 [r6] (raw file):
These unwrap() operations here start throwing me compiler errors saying that I cannot move out of borrowed content. But I do own these Options...

---

Comments from the review on Reviewable.io

[jdm]

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

---

Reviewed 1 of 1 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, 3 of 3 files at r6.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

components/net/fetch/request.rs, line 154 [r1] (raw file):
In case it was unclear, I meant "get the last element of the url list".

---

components/net/fetch/request.rs, line 215 [r3] (raw file):
I think we can get away with &self.url_list.last().as_ref().unwrap().scheme?

---

components/net/fetch/request.rs, line 152 [r4] (raw file):
Why this change? I thought all invocations of fetch algorithm concepts were synchronous after the initial asynchronous one.

---

components/net/fetch/request.rs, line 342 [r4] (raw file):
Why this change?

---

components/net/fetch/request.rs, line 345 [r6] (raw file):
Ah, the problem is that we're dereferencing through the Rc, which is an immutable borrow. We can use Rc::try_unwrap to work around that, as long as there's only one strong reference to actual_response here.

---

components/net_traits/lib.rs, line 123 [r4] (raw file):
I'm not convinced this is necessary.

---

Comments from the review on Reviewable.io

[KiChjang]

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

components/net/fetch/request.rs, line 154 [r1] (raw file):
Ah, then ignore the new fetch_async method and the corresponding FetchKind enum.

---

components/net/fetch/request.rs, line 342 [r4] (raw file):
The spec actually had removed this match arm and instead moved the handling of 304 to http_network_or_cache_fetch

---

components/net_traits/lib.rs, line 123 [r4] (raw file):
I'll revert this.

---

Comments from the review on Reviewable.io

[KiChjang]

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

components/net/fetch/request.rs, line 215 [r3] (raw file):
The idea here is that self.url_list cannot be taken as an immutable reference, because the self.http_fetch call down there is a mutable reference, and both scopes clash with each other.

---

components/net/fetch/request.rs, line 152 [r4] (raw file):
main_fetch is actually very asynchronous (that's why I had so much trouble implementing it earlier this year). I imagine one the possible solution to that is to generate a channel while waiting for some of the steps to finish.

---

Comments from the review on Reviewable.io

[jdm]

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

---

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

---

components/net/fetch/request.rs, line 351 [r7] (raw file):
I think the ok() is unnecesary here.

---

components/net/fetch/request.rs, line 352 [r7] (raw file):
Here too.

---

components/net/fetch/request.rs, line 386 [r7] (raw file):
This should be using *response.borrow_mut() = ... to update the value inside refcell instead of creating a new one.

---

Comments from the review on Reviewable.io

[KiChjang]

Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

components/net/fetch/request.rs, line 351 [r7] (raw file):
Nope, wouldn't compile if I remove the ok(), because try_unwrap returns a Result.

---

components/net/fetch/request.rs, line 352 [r7] (raw file):
Ditto above.

---

Comments from the review on Reviewable.io

[jdm]

unwrap is a method defined on Result too, so I'm not sure what the issue is.

[KiChjang]

/home/keith/Workspace/servo/components/net/fetch/request.rs:351:76: 351:84 error: no method named `unwrap` found for type `core::result::Result<core::cell::RefCell<net_traits::Response>, alloc::rc::Rc<core::cell::RefCell<net_traits::Response>>>` in the current scope
/home/keith/Workspace/servo/components/net/fetch/request.rs:351         let mut actual_response = Rc::try_unwrap(actual_response.unwrap()).unwrap().into_inner();
                                                                                                                                           ^~~~~~~~
/home/keith/Workspace/servo/components/net/fetch/request.rs:351:76: 351:84 note: the method `unwrap` exists but the following trait bounds were not satisfied: `alloc::rc::Rc<core::cell::RefCell<net_traits::Response>> : core::fmt::Debug`

I guess this means Response has to #[derive(Debug)]?

[jdm]

Ah, I see - the last line of that is the reason why we can't use unwrap directly.

[jdm]

@bors-servo: r+

---

Reviewed 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 94888a5 has been approved by jdm

[bors-servo]

⌛ Testing commit 94888a5 with merge 88367b4...

[bors-servo]

💔 Test failed - linux-dev

[jdm]

@bors-servo: delegate+

[bors-servo]

✌️ @KiChjang can now approve this pull request

[jdm]

@bors-servo: r+

[bors-servo]

📌 Commit 7d3eb72 has been approved by jdm

[bors-servo]

⌛ Testing commit 7d3eb72 with merge a515fe3...

[bors-servo]

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