Auto merge of #10785 - frewsxcv:loaderrortype-nostring, r=jdm

Refactor `LoadErrorType` to not require a `String` for every type.

Some of the `LoadErrorType` like `LoadCancelled` don't need a `String`
associated with the type since the variant is self-explanatory.

There are some variants that don't need an associated `String`, but that
can be cleaned up in a later refactor. Also, `net_traits::NetworkError`
currently requires a `String`, but that can potentially also be
refactored away too.

<!-- 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/10785)
<!-- Reviewable:end -->

2 files changed, 64 insertions, 43 deletions

diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs
index 1a53e013ad5..fedc3196397 100644
--- a/components/net/http_loader.rs
+++ b/components/net/http_loader.rs
@@ -36,6 +36,7 @@ use std::borrow::ToOwned;
 use std::boxed::FnBox;
 use std::collections::HashSet;
 use std::error::Error;
+use std::fmt;
 use std::io::{self, Read, Write};
 use std::sync::mpsc::Sender;
 use std::sync::{Arc, RwLock};
@@ -155,11 +156,11 @@ fn load_for_consumer(load_data: LoadData,
                user_agent, &cancel_listener) {
         Err(error) => {
             match error.error {
-                LoadErrorType::ConnectionAborted => unreachable!(),
-                LoadErrorType::Ssl => send_error(error.url.clone(),
-                                                 NetworkError::SslValidation(error.url),
-                                                 start_chan),
-                _ => send_error(error.url, NetworkError::Internal(error.reason), start_chan)
+                LoadErrorType::ConnectionAborted { .. } => unreachable!(),
+                LoadErrorType::Ssl { .. } => send_error(error.url.clone(),
+                                                        NetworkError::SslValidation(error.url),
+                                                        start_chan),
+                _ => send_error(error.url, NetworkError::Internal(error.error.description().to_owned()), start_chan)
             }
         }
         Ok(mut load_response) => {
@@ -247,14 +248,15 @@ impl HttpRequestFactory for NetworkHttpRequestFactory {
             if let Some(&SslError::OpenSslErrors(ref errors)) = error.downcast_ref::<SslError>() {
                 if errors.iter().any(is_cert_verify_error) {
                     let msg = format!("ssl error: {:?} {:?}", error.description(), error.cause());
-                    return Err(LoadError::new(url, LoadErrorType::Ssl, msg));
+                    return Err(LoadError::new(url, LoadErrorType::Ssl { reason: msg }));
                 }
             }
         }
 
         let mut request = match connection {
             Ok(req) => req,
-            Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection, e.description().to_owned())),
+            Err(e) => return Err(
+                LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })),
         };
         *request.headers_mut() = headers;
 
@@ -279,23 +281,22 @@ impl HttpRequest for WrappedHttpRequest {
         let url = self.request.url.clone();
         let mut request_writer = match self.request.start() {
             Ok(streaming) => streaming,
-            Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection, e.description().to_owned())),
+            Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })),
         };
 
         if let Some(ref data) = *body {
             if let Err(e) = request_writer.write_all(&data) {
-                return Err(LoadError::new(url, LoadErrorType::Connection, e.description().to_owned()))
+                return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() }))
             }
         }
 
         let response = match request_writer.send() {
             Ok(w) => w,
             Err(HttpError::Io(ref io_error)) if io_error.kind() == io::ErrorKind::ConnectionAborted => {
-                return Err(LoadError::new(url, LoadErrorType::ConnectionAborted,
-                                          io_error.description().to_owned()));
+                let error_type = LoadErrorType::ConnectionAborted { reason: io_error.description().to_owned() };
+                return Err(LoadError::new(url, error_type));
             },
-            Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection,
-                                                e.description().to_owned())),
+            Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })),
         };
 
         Ok(WrappedHttpResponse { response: response })
@@ -306,15 +307,13 @@ impl HttpRequest for WrappedHttpRequest {
 pub struct LoadError {
     pub url: Url,
     pub error: LoadErrorType,
-    pub reason: String,
 }
 
 impl LoadError {
-    pub fn new(url: Url, error: LoadErrorType, reason: String) -> LoadError {
+    pub fn new(url: Url, error: LoadErrorType) -> LoadError {
         LoadError {
             url: url,
             error: error,
-            reason: reason,
         }
     }
 }
@@ -322,14 +321,39 @@ impl LoadError {
 #[derive(Eq, PartialEq, Debug)]
 pub enum LoadErrorType {
     Cancelled,
-    Connection,
-    ConnectionAborted,
-    Cors,
-    Decoding,
-    InvalidRedirect,
+    Connection { reason: String },
+    ConnectionAborted { reason: String },
+    // Preflight fetch inconsistent with main fetch
+    CorsPreflightFetchInconsistent,
+    Decoding { reason: String },
+    InvalidRedirect { reason: String },
     MaxRedirects(u32), // u32 indicates number of redirects that occurred
-    Ssl,
-    UnsupportedScheme,
+    RedirectLoop,
+    Ssl { reason: String },
+    UnsupportedScheme { scheme: String },
+}
+
+impl fmt::Display for LoadErrorType {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.description())
+    }
+}
+
+impl Error for LoadErrorType {
+    fn description(&self) -> &str {
+        match *self {
+            LoadErrorType::Cancelled => "load cancelled",
+            LoadErrorType::Connection { ref reason } => reason,
+            LoadErrorType::ConnectionAborted { ref reason } => reason,
+            LoadErrorType::CorsPreflightFetchInconsistent => "preflight fetch inconsistent with main fetch",
+            LoadErrorType::Decoding { ref reason } => reason,
+            LoadErrorType::InvalidRedirect { ref reason } => reason,
+            LoadErrorType::MaxRedirects(_) => "too many redirects",
+            LoadErrorType::RedirectLoop => "redirect loop",
+            LoadErrorType::Ssl { ref reason } => reason,
+            LoadErrorType::UnsupportedScheme { .. } => "unsupported url scheme",
+        }
+    }
 }
 
 fn set_default_accept_encoding(headers: &mut Headers) {
@@ -492,7 +516,7 @@ impl<R: HttpResponse> StreamedResponse<R> {
                 let result = GzDecoder::new(response);
                 match result {
                     Ok(response_decoding) => Ok(StreamedResponse::new(m, Decoder::Gzip(response_decoding))),
-                    Err(err) => Err(LoadError::new(m.final_url, LoadErrorType::Decoding, err.to_string())),
+                    Err(err) => Err(LoadError::new(m.final_url, LoadErrorType::Decoding { reason: err.to_string() })),
                 }
             }
             Some(Encoding::Deflate) => {
@@ -708,7 +732,7 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
                                               headers.clone()));
 
         if cancel_listener.is_cancelled() {
-            return Err(LoadError::new(connection_url.clone(), LoadErrorType::Cancelled, "load cancelled".to_owned()));
+            return Err(LoadError::new(connection_url.clone(), LoadErrorType::Cancelled));
         }
 
         let maybe_response = req.send(request_body);
@@ -724,8 +748,8 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
         response = match maybe_response {
             Ok(r) => r,
             Err(e) => {
-                if let LoadErrorType::ConnectionAborted = e.error {
-                    debug!("connection aborted ({:?}), possibly stale, trying new connection", e.reason);
+                if let LoadErrorType::ConnectionAborted { reason } = e.error {
+                    debug!("connection aborted ({:?}), possibly stale, trying new connection", reason);
                     continue;
                 } else {
                     return Err(e)
@@ -777,7 +801,7 @@ pub fn load<A, B>(load_data: &LoadData,
     let mut new_auth_header: Option<Authorization<Basic>> = None;
 
     if cancel_listener.is_cancelled() {
-        return Err(LoadError::new(doc_url, LoadErrorType::Cancelled, "load cancelled".to_owned()));
+        return Err(LoadError::new(doc_url, LoadErrorType::Cancelled));
     }
 
     // If the URL is a view-source scheme then the scheme data contains the
@@ -799,17 +823,16 @@ pub fn load<A, B>(load_data: &LoadData,
         }
 
         if iters > max_redirects {
-            return Err(LoadError::new(doc_url, LoadErrorType::MaxRedirects(iters - 1),
-                                      "too many redirects".to_owned()));
+            return Err(LoadError::new(doc_url, LoadErrorType::MaxRedirects(iters - 1)));
         }
 
         if !matches!(doc_url.scheme(), "http" | "https") {
-            let s = format!("{} request, but we don't support that scheme", doc_url.scheme());
-            return Err(LoadError::new(doc_url, LoadErrorType::UnsupportedScheme, s));
+            let scheme = doc_url.scheme().to_owned();
+            return Err(LoadError::new(doc_url, LoadErrorType::UnsupportedScheme { scheme: scheme }));
         }
 
         if cancel_listener.is_cancelled() {
-            return Err(LoadError::new(doc_url, LoadErrorType::Cancelled, "load cancelled".to_owned()));
+            return Err(LoadError::new(doc_url, LoadErrorType::Cancelled));
         }
 
         info!("requesting {}", doc_url);
@@ -875,9 +898,7 @@ pub fn load<A, B>(load_data: &LoadData,
                 // CORS (https://fetch.spec.whatwg.org/#http-fetch, status section, point 9, 10)
                 if let Some(ref c) = load_data.cors {
                     if c.preflight {
-                        return Err(LoadError::new(doc_url,
-                                                  LoadErrorType::Cors,
-                                                  "Preflight fetch inconsistent with main fetch".to_owned()));
+                        return Err(LoadError::new(doc_url, LoadErrorType::CorsPreflightFetchInconsistent));
                     } else {
                         // XXXManishearth There are some CORS-related steps here,
                         // but they don't seem necessary until credentials are implemented
@@ -886,7 +907,8 @@ pub fn load<A, B>(load_data: &LoadData,
 
                 let new_doc_url = match doc_url.join(&new_url) {
                     Ok(u) => u,
-                    Err(e) => return Err(LoadError::new(doc_url, LoadErrorType::InvalidRedirect, e.to_string())),
+                    Err(e) => return Err(
+                        LoadError::new(doc_url, LoadErrorType::InvalidRedirect { reason: e.to_string() })),
                 };
 
                 // According to https://tools.ietf.org/html/rfc7231#section-6.4.2,
@@ -898,7 +920,7 @@ pub fn load<A, B>(load_data: &LoadData,
                 }
 
                 if redirected_to.contains(&new_doc_url) {
-                    return Err(LoadError::new(doc_url, LoadErrorType::InvalidRedirect, "redirect loop".to_owned()));
+                    return Err(LoadError::new(doc_url, LoadErrorType::RedirectLoop));
                 }
 
                 info!("redirecting to {}", new_doc_url);
diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs
index 83d21a66742..a32df4781bf 100644
--- a/tests/unit/net/http_loader.rs
+++ b/tests/unit/net/http_loader.rs
@@ -1082,8 +1082,7 @@ fn test_load_errors_when_there_a_redirect_loop() {
 
     match load(&load_data, &ui_provider, &http_state, None, &Factory,
                DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) {
-        Err(ref load_err) if load_err.error == LoadErrorType::InvalidRedirect =>
-            assert_eq!(&load_err.reason, "redirect loop"),
+        Err(ref load_err) if load_err.error == LoadErrorType::RedirectLoop => (),
         _ => panic!("expected max redirects to fail")
     }
 }
@@ -1172,7 +1171,7 @@ impl HttpRequestFactory for DontConnectFactory {
     type R = MockRequest;
 
     fn create(&self, url: Url, _: Method, _: Headers) -> Result<MockRequest, LoadError> {
-        Err(LoadError::new(url, LoadErrorType::Connection, "should not have connected".to_owned()))
+        Err(LoadError::new(url, LoadErrorType::Connection { reason: "should not have connected".into() }))
     }
 }
 
@@ -1190,7 +1189,7 @@ fn test_load_errors_when_scheme_is_not_http_or_https() {
                &DontConnectFactory,
                DEFAULT_USER_AGENT.to_owned(),
                &CancellationListener::new(None)) {
-        Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme => (),
+        Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme { scheme: "ftp".into() } => (),
         _ => panic!("expected ftp scheme to be unsupported")
     }
 }
@@ -1209,7 +1208,7 @@ fn test_load_errors_when_viewing_source_and_inner_url_scheme_is_not_http_or_http
                &DontConnectFactory,
                DEFAULT_USER_AGENT.to_owned(),
                &CancellationListener::new(None)) {
-        Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme => (),
+        Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme { scheme: "ftp".into() } => (),
         _ => panic!("expected ftp scheme to be unsupported")
     }
 }