Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update fetch methods #17521

Merged
merged 10 commits into from Aug 19, 2017

Update HTTP-redirect fetch

  • Loading branch information
KiChjang committed Aug 18, 2017
commit d6c197b40c216916eb0af860e05ff552c5907386
@@ -650,29 +650,20 @@ pub fn http_redirect_fetch(request: &mut Request,
// Step 1
assert!(response.return_internal);

// Step 2
if !response.actual_response().headers.has::<Location>() {
return response;
}

// Step 3
let location = match response.actual_response().headers.get::<Location>() {
Some(&Location(ref location)) => location.clone(),
_ => return Response::network_error(NetworkError::Internal("Location header parsing failure".into()))
};
let response_url = response.actual_response().url().unwrap();
let location_url = response_url.join(&*location);
let location_url = response.actual_response().location_url.clone();
let location_url = match location_url {
Ok(url) => url,
_ => return Response::network_error(NetworkError::Internal("Location URL parsing failure".into()))
// Step 2
None => return response,
// Step 3
Some(Err(err)) =>
return Response::network_error(
NetworkError::Internal("Location URL parse failure: ".to_owned() + &err)),
// Step 4
Some(Ok(ref url)) if !matches!(url.scheme(), "http" | "https") =>
return Response::network_error(NetworkError::Internal("Location URL not an HTTP(S) scheme".into())),
Some(Ok(url)) => url,
};

// Step 4
match location_url.scheme() {
"http" | "https" => { },
_ => return Response::network_error(NetworkError::Internal("Not an HTTP(S) Scheme".into()))
}

// Step 5
if request.redirect_count >= 20 {
return Response::network_error(NetworkError::Internal("Too many redirects".into()));
@@ -682,7 +673,8 @@ pub fn http_redirect_fetch(request: &mut Request,
request.redirect_count += 1;

// Step 7
let same_origin = location_url.origin()== request.current_url().origin();
// FIXME: Correctly use request's origin
let same_origin = location_url.origin() == request.current_url().origin();

This comment has been minimized.

Copy link
@jdm

jdm Aug 7, 2017

Member

Let's use the correct origin here. We can create an ImmutableOrigin value from the url's origin and compare them.

This comment has been minimized.

Copy link
@KiChjang

KiChjang Aug 8, 2017

Author Member

That shouldn't be neccessary either. All code paths that come from RequestInit should already have an ImmutableOrigin set for them.

let has_credentials = has_credentials(&location_url);

if request.mode == RequestMode::CorsMode && !same_origin && has_credentials {
@@ -695,28 +687,38 @@ pub fn http_redirect_fetch(request: &mut Request,
}

// Step 9
if response.actual_response().status.map_or(true, |s| s != StatusCode::SeeOther) &&
request.body.as_ref().map_or(false, |b| b.is_empty()) {
return Response::network_error(NetworkError::Internal("Request body is not done".into()));
}

// Step 10
if cors_flag && !same_origin {
request.origin = Origin::Origin(ImmutableOrigin::new_opaque());
}

// Step 10
let status_code = response.actual_response().status.unwrap();
if ((status_code == StatusCode::MovedPermanently || status_code == StatusCode::Found) &&
request.method == Method::Post) ||
status_code == StatusCode::SeeOther {
// Step 11
if response.actual_response().status.map_or(false, |code|
((code == StatusCode::MovedPermanently || code == StatusCode::Found) && request.method == Method::Post) ||
code == StatusCode::SeeOther) {

This comment has been minimized.

Copy link
@jdm

jdm Aug 7, 2017

Member

I don't think that this change is an improvement. I found it easier to read the code before.

This comment has been minimized.

Copy link
@KiChjang

KiChjang Aug 8, 2017

Author Member

The improvement here is that the code doesn't panic when it encounters a None on the status field of the actual response.

request.method = Method::Get;
request.body = None;
}

// Step 11
// Step 12
if let Some(_) = request.body {
// TODO: extract request's body's source
}

// Step 13
request.url_list.push(location_url);

// Step 12
// Step 14
// TODO implement referrer policy

// Step 15
let recursive_flag = request.redirect_mode != RedirectMode::Manual;

// Step 13
main_fetch(request, cache, cors_flag, recursive_flag, target, done_chan, context)
}

@@ -220,7 +220,7 @@ pub struct Request {
// TODO: target browsing context
/// https://fetch.spec.whatwg.org/#request-keepalive-flag
pub keep_alive: bool,
// https://fetch.spec.whatwg.org/#request-service-workers-mode
/// https://fetch.spec.whatwg.org/#request-service-workers-mode
pub service_workers_mode: ServiceWorkersMode,
/// https://fetch.spec.whatwg.org/#concept-request-initiator
pub initiator: Initiator,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.