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

Make SSL tests work #15784

Merged
merged 7 commits into from Apr 6, 2017

Remove unnecessary NetworkHttpRequestFactory abstraction.

  • Loading branch information
jdm committed Apr 6, 2017
commit ba132e0b4ce29dfb5b1bf6a23cbf7ae5bdd6ca20
@@ -15,7 +15,6 @@ use hsts::HstsList;
use hyper::Error as HttpError;
use hyper::LanguageTag;
use hyper::client::{Pool, Request as HyperRequest, Response as HyperResponse};
use hyper::client::pool::PooledStream;
use hyper::header::{Accept, AccessControlAllowCredentials, AccessControlAllowHeaders};
use hyper::header::{AccessControlAllowMethods, AccessControlAllowOrigin};
use hyper::header::{AccessControlMaxAge, AccessControlRequestHeaders};
@@ -27,9 +26,7 @@ use hyper::header::{IfUnmodifiedSince, IfModifiedSince, IfNoneMatch, Location};
use hyper::header::{Pragma, Quality, QualityItem, Referer, SetCookie};
use hyper::header::{UserAgent, q, qitem};
use hyper::method::Method;
use hyper::net::{Fresh, HttpStream, HttpsStream, NetworkConnector};
use hyper::status::StatusCode;
use hyper_openssl::SslStream;
use hyper_serde::Serde;
use log;
use msg::constellation_msg::PipelineId;
@@ -120,29 +117,6 @@ impl WrappedHttpResponse {
}
}

struct NetworkHttpRequestFactory {
pub connector: Arc<Pool<Connector>>,
}

impl NetworkConnector for NetworkHttpRequestFactory {
type Stream = PooledStream<HttpsStream<SslStream<HttpStream>>>;

fn connect(&self, host: &str, port: u16, scheme: &str) -> Result<Self::Stream, HttpError> {
self.connector.connect(host, port, scheme)
}
}

impl NetworkHttpRequestFactory {
fn create(&self, url: ServoUrl, method: Method, headers: Headers)
-> Result<HyperRequest<Fresh>, NetworkError> {
let connection = HyperRequest::with_connector(method, url.clone().into_url(), self);
let mut request = connection.map_err(|e| NetworkError::from_hyper_error(&url, e))?;
*request.headers_mut() = headers;

Ok(request)
}
}

// Step 3 of https://fetch.spec.whatwg.org/#concept-fetch.
pub fn set_default_accept(type_: Type, destination: Destination, headers: &mut Headers) {
if headers.has::<Accept>() {
@@ -415,7 +389,7 @@ fn auth_from_cache(auth_cache: &RwLock<AuthCache>, origin: &ImmutableOrigin) ->
}
}

fn obtain_response(request_factory: &NetworkHttpRequestFactory,
fn obtain_response(connector: Arc<Pool<Connector>>,
url: &ServoUrl,
method: &Method,
request_headers: &Headers,
@@ -466,8 +440,14 @@ fn obtain_response(request_factory: &NetworkHttpRequestFactory,

let connect_start = precise_time_ms();

let request = try!(request_factory.create(url.clone(), method.clone(),
headers.clone()));
let request = HyperRequest::with_connector(method.clone(),
url.clone().into_url(),
&*connector);
let mut request = match request {
Ok(request) => request,
Err(e) => return Err(NetworkError::from_hyper_error(&url, e)),
};
*request.headers_mut() = headers.clone();

let connect_end = precise_time_ms();

@@ -1089,10 +1069,6 @@ fn http_network_fetch(request: &Request,
// TODO be able to tell if the connection is a failure

// Step 4
let factory = NetworkHttpRequestFactory {
connector: context.connector.clone(),
};

let url = request.current_url();

let request_id = context.devtools_chan.as_ref().map(|_| {
@@ -1103,7 +1079,7 @@ fn http_network_fetch(request: &Request,
// do not. Once we support other kinds of fetches we'll need to be more fine grained here
// since things like image fetches are classified differently by devtools
let is_xhr = request.destination == Destination::None;
let wrapped_response = obtain_response(&factory, &url, &request.method,
let wrapped_response = obtain_response(context.connector.clone(), &url, &request.method,

This comment has been minimized.

Copy link
@nox

nox Apr 6, 2017

Member

AFAIK this clone is pointless, and you can pass &Pool<Connector> to obtain_response.

This comment has been minimized.

Copy link
@jdm

jdm Apr 6, 2017

Author Member

That was what I tried originally, and Rust's type checker became so confused that I didn't want to try to debug it. Please don't make me investigate it; it ends up trying to use the closure connector feature instead and it's not worth it.

&request.headers,
&request.body, &request.method,
&request.pipeline_id, request.redirect_count + 1,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.