Auto merge of #10555 - jdm:http_response_refactor, r=ms2ger

Remove unnecessary indirection in HTTP unit tests

Many of the HTTP unit tests use a custom request factory as well as a custom request that contained test logic. This is unnecessarily convoluted, and exists solely because the complete set of headers was unavailable until the request body was sent. These patches restructure the header manipulations so that the headers are available when the request object is created, allowing the test logic to move into the test factories, and enabling the deletion of almost all of the test request types.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10555)
<!-- Reviewable:end -->

components/net/http_loader.rs

@@ -154,9 +154,9 @@ fn load_for_consumer(load_data: LoadData,
 
     let ui_provider = TFDProvider;
     let context = load_data.context.clone();
-    match load::<WrappedHttpRequest, TFDProvider>(load_data, &ui_provider, &http_state,
-                                     devtools_chan, &factory,
-                                     user_agent, &cancel_listener) {
+    match load(load_data, &ui_provider, &http_state,
+               devtools_chan, &factory,
+               user_agent, &cancel_listener) {
         Err(LoadError::UnsupportedScheme(url)) => {
             let s = format!("{} request, but we don't support that scheme", &*url.scheme);
             send_error(url, s, start_chan)
@@ -249,7 +249,7 @@ impl HttpResponse for WrappedHttpResponse {
 pub trait HttpRequestFactory {
     type R: HttpRequest;
 
-    fn create(&self, url: Url, method: Method) -> Result<Self::R, LoadError>;
+    fn create(&self, url: Url, method: Method, headers: Headers) -> Result<Self::R, LoadError>;
 }
 
 pub struct NetworkHttpRequestFactory {
@@ -259,7 +259,8 @@ pub struct NetworkHttpRequestFactory {
 impl HttpRequestFactory for NetworkHttpRequestFactory {
     type R = WrappedHttpRequest;
 
-    fn create(&self, url: Url, method: Method) -> Result<WrappedHttpRequest, LoadError> {
+    fn create(&self, url: Url, method: Method, headers: Headers)
+              -> Result<WrappedHttpRequest, LoadError> {
         let connection = Request::with_connector(method, url.clone(), &*self.connector);
 
         if let Err(HttpError::Ssl(ref error)) = connection {
@@ -274,13 +275,14 @@ impl HttpRequestFactory for NetworkHttpRequestFactory {
             }
         }
 
-        let request = match connection {
+        let mut request = match connection {
             Ok(req) => req,
 
             Err(e) => {
                  return Err(LoadError::Connection(url, e.description().to_owned()))
             }
         };
+        *request.headers_mut() = headers;
 
         Ok(WrappedHttpRequest { request: request })
     }
@@ -289,7 +291,6 @@ impl HttpRequestFactory for NetworkHttpRequestFactory {
 pub trait HttpRequest {
     type R: HttpResponse + 'static;
 
-    fn headers_mut(&mut self) -> &mut Headers;
     fn send(self, body: &Option<Vec<u8>>) -> Result<Self::R, LoadError>;
 }
 
@@ -300,10 +301,6 @@ pub struct WrappedHttpRequest {
 impl HttpRequest for WrappedHttpRequest {
     type R = WrappedHttpResponse;
 
-    fn headers_mut(&mut self) -> &mut Headers {
-        self.request.headers_mut()
-    }
-
     fn send(self, body: &Option<Vec<u8>>) -> Result<WrappedHttpResponse, LoadError> {
         let url = self.request.url.clone();
         let mut request_writer = match self.request.start() {
@@ -628,6 +625,7 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
                           request_id: &str)
                           -> Result<A::R, LoadError> where A: HttpRequest + 'static  {
 
+    let null_data = None;
     let response;
     let connection_url = replace_hosts(&url);
 
@@ -636,20 +634,7 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
     // a ConnectionAborted error. this loop tries again with a new
     // connection.
     loop {
-        let mut req = try!(request_factory.create(connection_url.clone(), method.clone()));
-        *req.headers_mut() = request_headers.clone();
-
-        if cancel_listener.is_cancelled() {
-            return Err(LoadError::Cancelled(connection_url.clone(), "load cancelled".to_owned()));
-        }
-
-        if log_enabled!(log::LogLevel::Info) {
-            info!("{}", method);
-            for header in req.headers_mut().iter() {
-                info!(" - {}", header);
-            }
-            info!("{:?}", data);
-        }
+        let mut headers = request_headers.clone();
 
         // Avoid automatically sending request body if a redirect has occurred.
         //
@@ -658,26 +643,42 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
         //
         // https://tools.ietf.org/html/rfc7231#section-6.4
         let is_redirected_request = iters != 1;
-        let cloned_data;
-        let maybe_response = match data {
+        let request_body;
+        match data {
             &Some(ref d) if !is_redirected_request => {
-                req.headers_mut().set(ContentLength(d.len() as u64));
-                cloned_data = data.clone();
-                req.send(data)
-            },
+                headers.set(ContentLength(d.len() as u64));
+                request_body = data;
+            }
             _ => {
                 if *load_data_method != Method::Get && *load_data_method != Method::Head {
-                    req.headers_mut().set(ContentLength(0))
+                    headers.set(ContentLength(0))
                 }
-                cloned_data = None;
-                req.send(&None)
+                request_body = &null_data;
             }
-        };
+        }
+
+        if log_enabled!(log::LogLevel::Info) {
+            info!("{}", method);
+            for header in headers.iter() {
+                info!(" - {}", header);
+            }
+            info!("{:?}", data);
+        }
+
+        let req = try!(request_factory.create(connection_url.clone(), method.clone(),
+                                              headers.clone()));
+
+        if cancel_listener.is_cancelled() {
+            return Err(LoadError::Cancelled(connection_url.clone(), "load cancelled".to_owned()));
+        }
+
+        let maybe_response = req.send(request_body);
+
         if let Some(pipeline_id) = *pipeline_id {
             send_request_to_devtools(
                 devtools_chan.clone(), request_id.clone().into(),
-                url.clone(), method.clone(), request_headers.clone(),
-                cloned_data, pipeline_id, time::now()
+                url.clone(), method.clone(), headers,
+                request_body.clone(), pipeline_id, time::now()
             );
         }