From dc790048ecb57152128b110f7f1c3e3925f4bfee Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 17:32:45 -0400 Subject: [PATCH 01/11] Remove unnecessary type annotations. --- components/net/http_loader.rs | 6 +- tests/unit/net/http_loader.rs | 270 +++++++++++++++++----------------- 2 files changed, 138 insertions(+), 138 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 6578a83b8c2..5f1d17bd010 100644 --- a/components/net/http_loader.rs +++ b/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::(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) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 10aa01d3e4c..d5f70bd3b90 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -474,22 +474,22 @@ fn test_check_default_headers_loaded_in_every_request() { headers.set(UserAgent(DEFAULT_USER_AGENT.to_owned())); // Testing for method.GET - let _ = load::(load_data.clone(), &ui_provider, &http_state, None, - &AssertMustHaveHeadersRequestFactory { - expected_headers: headers.clone(), - body: <[_]>::to_vec(&[]) - }, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); + let _ = load(load_data.clone(), &ui_provider, &http_state, None, + &AssertMustHaveHeadersRequestFactory { + expected_headers: headers.clone(), + body: <[_]>::to_vec(&[]) + }, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); // Testing for method.POST load_data.method = Method::Post; headers.set(ContentLength(0 as u64)); - let _ = load::(load_data.clone(), &ui_provider, &http_state, None, - &AssertMustHaveHeadersRequestFactory { - expected_headers: headers, - body: <[_]>::to_vec(&[]) - }, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); + let _ = load(load_data.clone(), &ui_provider, &http_state, None, + &AssertMustHaveHeadersRequestFactory { + expected_headers: headers, + body: <[_]>::to_vec(&[]) + }, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); } #[test] @@ -506,7 +506,7 @@ fn test_load_when_request_is_not_get_or_head_and_there_is_no_body_content_length let mut content_length = Headers::new(); content_length.set(ContentLength(0)); - let _ = load::( + let _ = load( load_data.clone(), &ui_provider, &http_state, None, &AssertMustIncludeHeadersRequestFactory { expected_headers: content_length, @@ -541,8 +541,8 @@ fn test_request_and_response_data_with_network_messages() { let mut request_headers = Headers::new(); request_headers.set(Host { hostname: "bar.foo".to_owned(), port: None }); load_data.headers = request_headers.clone(); - let _ = load::(load_data, &ui_provider, &http_state, Some(devtools_chan), &Factory, - DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); + let _ = load(load_data, &ui_provider, &http_state, Some(devtools_chan), &Factory, + DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); // notification received from devtools let devhttprequest = expect_devtools_http_request(&devtools_port); @@ -611,8 +611,8 @@ fn test_request_and_response_message_from_devtool_without_pipeline_id() { let url = Url::parse("https://mozilla.com").unwrap(); let (devtools_chan, devtools_port) = mpsc::channel::(); let load_data = LoadData::new(LoadContext::Browsing, url.clone(), None); - let _ = load::(load_data, &ui_provider, &http_state, Some(devtools_chan), &Factory, - DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); + let _ = load(load_data, &ui_provider, &http_state, Some(devtools_chan), &Factory, + DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); // notification received from devtools assert!(devtools_port.try_recv().is_err()); @@ -645,8 +645,8 @@ fn test_load_when_redirecting_from_a_post_should_rewrite_next_request_as_get() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, &ui_provider, &http_state, None, &Factory, - DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); + let _ = load(load_data, &ui_provider, &http_state, None, &Factory, + DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)); } #[test] @@ -673,7 +673,7 @@ fn test_load_should_decode_the_response_as_deflate_when_response_headers_have_co let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let mut response = load::( + let mut response = load( load_data, &ui_provider, &http_state, None, &Factory, DEFAULT_USER_AGENT.to_owned(), @@ -706,7 +706,7 @@ fn test_load_should_decode_the_response_as_gzip_when_response_headers_have_conte let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let mut response = load::( + let mut response = load( load_data, &ui_provider, &http_state, None, &Factory, @@ -750,7 +750,7 @@ fn test_load_doesnt_send_request_body_on_any_redirect() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::( + let _ = load( load_data, &ui_provider, &http_state, None, &Factory, @@ -780,12 +780,12 @@ fn test_load_doesnt_add_host_to_sts_list_when_url_is_http_even_if_sts_headers_ar let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); assert_eq!(http_state.hsts_list.read().unwrap().is_host_secure("mozilla.com"), false); } @@ -812,12 +812,12 @@ fn test_load_adds_host_to_sts_list_when_url_is_https_and_sts_headers_are_present let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); assert!(http_state.hsts_list.read().unwrap().is_host_secure("mozilla.com")); } @@ -846,12 +846,12 @@ fn test_load_sets_cookies_in_the_resource_manager_when_it_get_set_cookie_header_ let load_data = LoadData::new(LoadContext::Browsing, url.clone(), None); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); assert_cookie_for_domain(http_state.cookie_jar.clone(), "http://mozilla.com", "mozillaIs=theBest"); } @@ -880,12 +880,12 @@ fn test_load_sets_requests_cookies_header_for_url_by_getting_cookies_from_the_re let mut cookie = Headers::new(); cookie.set(CookieHeader(vec![CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned())])); - let _ = load::(load_data.clone(), &ui_provider, &http_state, None, - &AssertMustIncludeHeadersRequestFactory { - expected_headers: cookie, - body: <[_]>::to_vec(&*load_data.data.unwrap()) - }, DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data.clone(), &ui_provider, &http_state, None, + &AssertMustIncludeHeadersRequestFactory { + expected_headers: cookie, + body: <[_]>::to_vec(&*load_data.data.unwrap()) + }, DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); } #[test] @@ -922,7 +922,7 @@ fn test_load_sends_secure_cookie_if_http_changed_to_https_due_to_entry_in_hsts_s let mut headers = Headers::new(); headers.set_raw("Cookie".to_owned(), vec![<[_]>::to_vec("mozillaIs=theBest".as_bytes())]); - let _ = load::( + let _ = load( load_data.clone(), &ui_provider, &http_state, None, &AssertMustIncludeHeadersRequestFactory { expected_headers: headers, @@ -954,7 +954,7 @@ fn test_load_sends_cookie_if_nonhttp() { let mut headers = Headers::new(); headers.set_raw("Cookie".to_owned(), vec![<[_]>::to_vec("mozillaIs=theBest".as_bytes())]); - let _ = load::( + let _ = load( load_data.clone(), &ui_provider, &http_state, None, &AssertMustIncludeHeadersRequestFactory { expected_headers: headers, @@ -983,12 +983,12 @@ fn test_cookie_set_with_httponly_should_not_be_available_using_getcookiesforurl( let ui_provider = TestProvider::new(); let load_data = LoadData::new(LoadContext::Browsing, url.clone(), None); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); let mut cookie_jar = http_state.cookie_jar.write().unwrap(); assert!(cookie_jar.cookies_for_url(&url, CookieSource::NonHTTP).is_none()); @@ -1013,12 +1013,12 @@ fn test_when_cookie_received_marked_secure_is_ignored_for_http() { let ui_provider = TestProvider::new(); let load_data = LoadData::new(LoadContext::Browsing, Url::parse("http://mozilla.com").unwrap(), None); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); assert_cookie_for_domain(http_state.cookie_jar.clone(), "http://mozilla.com", ""); } @@ -1048,7 +1048,7 @@ fn test_when_cookie_set_marked_httpsonly_secure_isnt_sent_on_http_request() { assert_cookie_for_domain(http_state.cookie_jar.clone(), "https://mozilla.com", "mozillaIs=theBest"); - let _ = load::( + let _ = load( load_data.clone(), &ui_provider, &http_state, None, &AssertMustNotIncludeHeadersRequestFactory { headers_not_expected: vec!["Cookie".to_owned()], @@ -1070,12 +1070,12 @@ fn test_load_sets_content_length_to_length_of_request_body() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data.clone(), &ui_provider, &http_state, - None, &AssertMustIncludeHeadersRequestFactory { - expected_headers: content_len_headers, - body: <[_]>::to_vec(&*load_data.data.unwrap()) - }, DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data.clone(), &ui_provider, &http_state, + None, &AssertMustIncludeHeadersRequestFactory { + expected_headers: content_len_headers, + body: <[_]>::to_vec(&*load_data.data.unwrap()) + }, DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); } #[test] @@ -1093,14 +1093,14 @@ fn test_load_uses_explicit_accept_from_headers_in_load_data() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &AssertMustIncludeHeadersRequestFactory { - expected_headers: accept_headers, - body: <[_]>::to_vec("Yay!".as_bytes()) - }, DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &AssertMustIncludeHeadersRequestFactory { + expected_headers: accept_headers, + body: <[_]>::to_vec("Yay!".as_bytes()) + }, DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); } #[test] @@ -1120,14 +1120,14 @@ fn test_load_sets_default_accept_to_html_xhtml_xml_and_then_anything_else() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &AssertMustIncludeHeadersRequestFactory { - expected_headers: accept_headers, - body: <[_]>::to_vec("Yay!".as_bytes()) - }, DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &AssertMustIncludeHeadersRequestFactory { + expected_headers: accept_headers, + body: <[_]>::to_vec("Yay!".as_bytes()) + }, DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); } #[test] @@ -1143,14 +1143,14 @@ fn test_load_uses_explicit_accept_encoding_from_load_data_headers() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &AssertMustIncludeHeadersRequestFactory { - expected_headers: accept_encoding_headers, - body: <[_]>::to_vec("Yay!".as_bytes()) - }, DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &AssertMustIncludeHeadersRequestFactory { + expected_headers: accept_encoding_headers, + body: <[_]>::to_vec("Yay!".as_bytes()) + }, DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); } #[test] @@ -1167,14 +1167,14 @@ fn test_load_sets_default_accept_encoding_to_gzip_and_deflate() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - let _ = load::(load_data, - &ui_provider, &http_state, - None, - &AssertMustIncludeHeadersRequestFactory { - expected_headers: accept_encoding_headers, - body: <[_]>::to_vec("Yay!".as_bytes()) - }, DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)); + let _ = load(load_data, + &ui_provider, &http_state, + None, + &AssertMustIncludeHeadersRequestFactory { + expected_headers: accept_encoding_headers, + body: <[_]>::to_vec("Yay!".as_bytes()) + }, DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)); } #[test] @@ -1201,8 +1201,8 @@ fn test_load_errors_when_there_a_redirect_loop() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, &ui_provider, &http_state, None, &Factory, - DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { + match load(load_data, &ui_provider, &http_state, None, &Factory, + DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { Err(LoadError::InvalidRedirect(_, msg)) => { assert_eq!(msg, "redirect loop"); }, @@ -1232,8 +1232,8 @@ fn test_load_errors_when_there_is_too_many_redirects() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, &ui_provider, &http_state, None, &Factory, - DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { + match load(load_data, &ui_provider, &http_state, None, &Factory, + DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { Err(LoadError::MaxRedirects(url)) => { assert_eq!(url.domain().unwrap(), "mozilla.com") }, @@ -1271,8 +1271,8 @@ fn test_load_follows_a_redirect() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, &ui_provider, &http_state, None, &Factory, - DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { + match load(load_data, &ui_provider, &http_state, None, &Factory, + DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { Err(e) => panic!("expected to follow a redirect {:?}", e), Ok(mut lr) => { let response = read_response(&mut lr); @@ -1299,12 +1299,12 @@ fn test_load_errors_when_scheme_is_not_http_or_https() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, - &ui_provider, &http_state, - None, - &DontConnectFactory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)) { + match load(load_data, + &ui_provider, &http_state, + None, + &DontConnectFactory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)) { Err(LoadError::UnsupportedScheme(_)) => {} _ => panic!("expected ftp scheme to be unsupported") } @@ -1318,12 +1318,12 @@ fn test_load_errors_when_viewing_source_and_inner_url_scheme_is_not_http_or_http let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, - &ui_provider, &http_state, - None, - &DontConnectFactory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)) { + match load(load_data, + &ui_provider, &http_state, + None, + &DontConnectFactory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)) { Err(LoadError::UnsupportedScheme(_)) => {} _ => panic!("expected ftp scheme to be unsupported") } @@ -1360,12 +1360,12 @@ fn test_load_errors_when_cancelled() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &cancel_listener) { + match load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &cancel_listener) { Err(LoadError::Cancelled(_, _)) => (), _ => panic!("expected load cancelled error!") } @@ -1428,12 +1428,12 @@ fn test_redirect_from_x_to_y_provides_y_cookies_from_y() { cookie_jar.push(cookie_y, CookieSource::HTTP); } - match load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)) { + match load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)) { Err(e) => panic!("expected to follow a redirect {:?}", e), Ok(mut lr) => { let response = read_response(&mut lr); @@ -1476,12 +1476,12 @@ fn test_redirect_from_x_to_x_provides_x_with_cookie_from_first_response() { let http_state = HttpState::new(); let ui_provider = TestProvider::new(); - match load::(load_data, - &ui_provider, &http_state, - None, - &Factory, - DEFAULT_USER_AGENT.to_owned(), - &CancellationListener::new(None)) { + match load(load_data, + &ui_provider, &http_state, + None, + &Factory, + DEFAULT_USER_AGENT.to_owned(), + &CancellationListener::new(None)) { Err(e) => panic!("expected to follow a redirect {:?}", e), Ok(mut lr) => { let response = read_response(&mut lr); @@ -1518,7 +1518,7 @@ fn test_if_auth_creds_not_in_url_but_in_cache_it_sets_it() { ) ); - let _ = load::( + let _ = load( load_data.clone(), &ui_provider, &http_state, None, &AssertMustIncludeHeadersRequestFactory { expected_headers: auth_header, @@ -1545,7 +1545,7 @@ fn test_auth_ui_sets_header_on_401() { let load_data = LoadData::new(LoadContext::Browsing, url, None); - match load::( + match load( load_data.clone(), &ui_provider, &http_state, None, &AssertAuthHeaderRequestFactory { expected_headers: auth_header, From 80eaeac9f4a2295b21a204a67ec8ce5b399a1514 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 17:44:14 -0400 Subject: [PATCH 02/11] Make headers available when creating an HTTP request via a factory. --- components/net/http_loader.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 5f1d17bd010..64ca63d2e37 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -249,7 +249,16 @@ impl HttpResponse for WrappedHttpResponse { pub trait HttpRequestFactory { type R: HttpRequest; - fn create(&self, url: Url, method: Method) -> Result; + fn create(&self, _url: Url, _method: Method) -> Result { + panic!() + } + fn create_with_headers(&self, url: Url, method: Method, headers: Headers) -> Result { + let mut result = self.create(url, method); + if let Ok(ref mut req) = result { + *req.headers_mut() = headers; + } + result + } } pub struct NetworkHttpRequestFactory { @@ -636,8 +645,8 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, // 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(); + let mut req = try!(request_factory.create_with_headers(connection_url.clone(), method.clone(), + request_headers.clone())); if cancel_listener.is_cancelled() { return Err(LoadError::Cancelled(connection_url.clone(), "load cancelled".to_owned())); From 0e1703d747ad818af32a126dae86251e8d6144c4 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 17:46:19 -0400 Subject: [PATCH 03/11] Convert NetworkHttpRequestFactory. --- components/net/http_loader.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 64ca63d2e37..fd6f98c7c3d 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -268,7 +268,8 @@ pub struct NetworkHttpRequestFactory { impl HttpRequestFactory for NetworkHttpRequestFactory { type R = WrappedHttpRequest; - fn create(&self, url: Url, method: Method) -> Result { + fn create_with_headers(&self, url: Url, method: Method, headers: Headers) + -> Result { let connection = Request::with_connector(method, url.clone(), &*self.connector); if let Err(HttpError::Ssl(ref error)) = connection { @@ -283,13 +284,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 }) } From a315f8db9ec080b00eb9fb3334bbd826967c198a Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:05:28 -0400 Subject: [PATCH 04/11] Reorganize header manipulation that occurs before sending an HTTP request so we can provide the full set of headers while creating a request. --- components/net/http_loader.rs | 56 +++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index fd6f98c7c3d..e8e0ad682e1 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -639,6 +639,7 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, request_id: &str) -> Result where A: HttpRequest + 'static { + let null_data = None; let response; let connection_url = replace_hosts(&url); @@ -647,20 +648,7 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, // a ConnectionAborted error. this loop tries again with a new // connection. loop { - let mut req = try!(request_factory.create_with_headers(connection_url.clone(), method.clone(), - 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. // @@ -669,26 +657,42 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, // // 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_with_headers(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() ); } From dec66b2215e4536071a5454a30158d13f88127c2 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:06:37 -0400 Subject: [PATCH 05/11] Convert AssertMustHaveHeadersRequestFactory. --- tests/unit/net/http_loader.rs | 35 ++++------------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index d5f70bd3b90..fde10ff00f0 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -179,29 +179,6 @@ impl HttpRequest for MockRequest { } } -struct AssertRequestMustHaveHeaders { - expected_headers: Headers, - request_headers: Headers, - t: ResponseType -} - -impl AssertRequestMustHaveHeaders { - fn new(t: ResponseType, expected_headers: Headers) -> Self { - AssertRequestMustHaveHeaders { expected_headers: expected_headers, request_headers: Headers::new(), t: t } - } -} - -impl HttpRequest for AssertRequestMustHaveHeaders { - type R = MockResponse; - - fn headers_mut(&mut self) -> &mut Headers { &mut self.request_headers } - - fn send(self, _: &Option>) -> Result { - assert_eq!(self.request_headers, self.expected_headers); - response_for_request_type(self.t) - } -} - struct AssertAuthHeaderRequest { expected_headers: Headers, request_headers: Headers, @@ -285,15 +262,11 @@ struct AssertMustHaveHeadersRequestFactory { } impl HttpRequestFactory for AssertMustHaveHeadersRequestFactory { - type R = AssertRequestMustHaveHeaders; + type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { - Ok( - AssertRequestMustHaveHeaders::new( - ResponseType::Text(self.body.clone()), - self.expected_headers.clone() - ) - ) + fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + assert_eq!(headers, self.expected_headers); + Ok(MockRequest::new(ResponseType::Text(self.body.clone()))) } } From 36de07bfdfc6140b0c45f60412766fa4ba3b8cf0 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:18:37 -0400 Subject: [PATCH 06/11] Convert AssertMustIncludeHeadersRequestFactory. --- tests/unit/net/http_loader.rs | 92 ++++++++++------------------------- 1 file changed, 27 insertions(+), 65 deletions(-) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index fde10ff00f0..e21a7c524b7 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -213,49 +213,6 @@ impl HttpRequest for AssertAuthHeaderRequest { } } -struct AssertRequestMustIncludeHeaders { - expected_headers: Headers, - request_headers: Headers, - t: ResponseType -} - -impl AssertRequestMustIncludeHeaders { - fn new(t: ResponseType, expected_headers_op: Option) -> Self { - match expected_headers_op { - Some(expected_headers) => { - assert!(expected_headers.len() != 0); - AssertRequestMustIncludeHeaders { - expected_headers: expected_headers, - request_headers: Headers::new(), - t: t - } - } - None => AssertRequestMustIncludeHeaders { - expected_headers: Headers::new(), - request_headers: Headers::new(), - t: t - } - } - } -} - -impl HttpRequest for AssertRequestMustIncludeHeaders { - type R = MockResponse; - - fn headers_mut(&mut self) -> &mut Headers { &mut self.request_headers } - - fn send(self, _: &Option>) -> Result { - for header in self.expected_headers.iter() { - assert!(self.request_headers.get_raw(header.name()).is_some()); - assert_eq!( - self.request_headers.get_raw(header.name()).unwrap(), - self.expected_headers.get_raw(header.name()).unwrap() - ) - } - response_for_request_type(self.t) - } -} - struct AssertMustHaveHeadersRequestFactory { expected_headers: Headers, body: Vec @@ -288,21 +245,26 @@ impl HttpRequestFactory for AssertAuthHeaderRequestFactory { } } +fn assert_headers_included(expected: &Headers, request: &Headers) { + assert!(expected.len() != 0); + for header in expected.iter() { + assert!(request.get_raw(header.name()).is_some()); + assert_eq!(request.get_raw(header.name()).unwrap(), + expected.get_raw(header.name()).unwrap()) + } +} + struct AssertMustIncludeHeadersRequestFactory { expected_headers: Headers, body: Vec } impl HttpRequestFactory for AssertMustIncludeHeadersRequestFactory { - type R = AssertRequestMustIncludeHeaders; + type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { - Ok( - AssertRequestMustIncludeHeaders::new( - ResponseType::Text(self.body.clone()), - Some(self.expected_headers.clone()) - ) - ) + fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + assert_headers_included(&self.expected_headers, &headers); + Ok(MockRequest::new(ResponseType::Text(self.body.clone()))) } } @@ -1352,24 +1314,26 @@ fn test_redirect_from_x_to_y_provides_y_cookies_from_y() { struct Factory; impl HttpRequestFactory for Factory { - type R = AssertRequestMustIncludeHeaders; + type R = MockRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, headers: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { let mut expected_headers_x = Headers::new(); expected_headers_x.set_raw("Cookie".to_owned(), vec![<[_]>::to_vec("mozillaIsNot=dotCom".as_bytes())]); + assert_headers_included(&expected_headers_x, &headers); - Ok(AssertRequestMustIncludeHeaders::new( - ResponseType::Redirect("http://mozilla.org".to_owned()), Some(expected_headers_x))) + Ok(MockRequest::new( + ResponseType::Redirect("http://mozilla.org".to_owned()))) } else if url.domain().unwrap() == "mozilla.org" { let mut expected_headers_y = Headers::new(); expected_headers_y.set_raw( "Cookie".to_owned(), vec![<[_]>::to_vec("mozillaIs=theBest".as_bytes())]); + assert_headers_included(&expected_headers_y, &headers); - Ok(AssertRequestMustIncludeHeaders::new( - ResponseType::Text(<[_]>::to_vec("Yay!".as_bytes())), Some(expected_headers_y))) + Ok(MockRequest::new( + ResponseType::Text(<[_]>::to_vec("Yay!".as_bytes())))) } else { panic!("unexpected host {:?}", url) } @@ -1422,22 +1386,20 @@ fn test_redirect_from_x_to_x_provides_x_with_cookie_from_first_response() { struct Factory; impl HttpRequestFactory for Factory { - type R = AssertRequestMustIncludeHeaders; + type R = MockRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, headers: Headers) -> Result { if url.path().unwrap()[0] == "initial" { let mut initial_answer_headers = Headers::new(); initial_answer_headers.set_raw("set-cookie", vec![b"mozillaIs=theBest; path=/;".to_vec()]); - Ok(AssertRequestMustIncludeHeaders::new( + Ok(MockRequest::new( ResponseType::RedirectWithHeaders("http://mozilla.org/subsequent/".to_owned(), - initial_answer_headers), - None)) + initial_answer_headers))) } else if url.path().unwrap()[0] == "subsequent" { let mut expected_subsequent_headers = Headers::new(); expected_subsequent_headers.set_raw("Cookie", vec![b"mozillaIs=theBest".to_vec()]); - Ok(AssertRequestMustIncludeHeaders::new( - ResponseType::Text(b"Yay!".to_vec()), - Some(expected_subsequent_headers))) + assert_headers_included(&expected_subsequent_headers, &headers); + Ok(MockRequest::new(ResponseType::Text(b"Yay!".to_vec()))) } else { panic!("unexpected host {:?}", url) } From f712ddb0db3bd776c1ccbb44d9d02236d7895ddc Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Fri, 15 Apr 2016 01:42:53 -0400 Subject: [PATCH 07/11] Convert AssertAuthHeaderRequestFactory. --- tests/unit/net/http_loader.rs | 63 ++++++++++------------------------- 1 file changed, 17 insertions(+), 46 deletions(-) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index e21a7c524b7..4dfd29c9b25 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -113,10 +113,9 @@ impl UIProvider for TestProvider { } } -fn basic_auth(headers: Headers) -> MockResponse { - +fn basic_auth() -> MockResponse { MockResponse::new( - headers, + Headers::new(), StatusCode::Unauthorized, RawStatus(401, Cow::Borrowed("Unauthorized")), b"".to_vec() @@ -138,7 +137,8 @@ enum ResponseType { Redirect(String), RedirectWithHeaders(String, Headers), Text(Vec), - WithHeaders(Vec, Headers) + WithHeaders(Vec, Headers), + NeedsAuth, } struct MockRequest { @@ -165,6 +165,9 @@ fn response_for_request_type(t: ResponseType) -> Result }, ResponseType::WithHeaders(b, h) => { Ok(respond_with_headers(b, h)) + }, + ResponseType::NeedsAuth => { + Ok(basic_auth()) } } } @@ -179,40 +182,6 @@ impl HttpRequest for MockRequest { } } -struct AssertAuthHeaderRequest { - expected_headers: Headers, - request_headers: Headers, - t: ResponseType -} - -impl AssertAuthHeaderRequest { - fn new(t: ResponseType, expected_headers: Headers) -> Self { - AssertAuthHeaderRequest { expected_headers: expected_headers, request_headers: Headers::new(), t: t } - } -} - -impl HttpRequest for AssertAuthHeaderRequest { - type R = MockResponse; - - fn headers_mut(&mut self) -> &mut Headers { &mut self.request_headers } - - fn send(self, _: &Option>) -> Result { - - if self.request_headers.has::>() { - for header in self.expected_headers.iter() { - assert!(self.request_headers.get_raw(header.name()).is_some()); - assert_eq!( - self.request_headers.get_raw(header.name()).unwrap(), - self.expected_headers.get_raw(header.name()).unwrap() - ) - } - response_for_request_type(self.t) - } else { - Ok(basic_auth(self.request_headers)) - } - } -} - struct AssertMustHaveHeadersRequestFactory { expected_headers: Headers, body: Vec @@ -233,15 +202,17 @@ struct AssertAuthHeaderRequestFactory { } impl HttpRequestFactory for AssertAuthHeaderRequestFactory { - type R = AssertAuthHeaderRequest; + type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { - Ok( - AssertAuthHeaderRequest::new( - ResponseType::Text(self.body.clone()), - self.expected_headers.clone() - ) - ) + fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + let request = if headers.has::>() { + assert_headers_included(&self.expected_headers, &headers); + MockRequest::new(ResponseType::Text(self.body.clone())) + } else { + MockRequest::new(ResponseType::NeedsAuth) + }; + + Ok(request) } } From d1b07673b8837f5f5dd97466a617853b576a865e Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:23:18 -0400 Subject: [PATCH 08/11] Convert AssertMustNotIncludeHeadersRequestFactory. --- tests/unit/net/http_loader.rs | 45 +++++++---------------------------- 1 file changed, 8 insertions(+), 37 deletions(-) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 4dfd29c9b25..8ac53cebc9f 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -251,50 +251,21 @@ fn assert_cookie_for_domain(cookie_jar: Arc>, domain: &str } } -struct AssertRequestMustNotIncludeHeaders { - headers_not_expected: Vec, - request_headers: Headers, - t: ResponseType -} - -impl AssertRequestMustNotIncludeHeaders { - fn new(t: ResponseType, headers_not_expected: Vec) -> Self { - assert!(headers_not_expected.len() != 0); - AssertRequestMustNotIncludeHeaders { - headers_not_expected: headers_not_expected, - request_headers: Headers::new(), t: t } - } -} - -impl HttpRequest for AssertRequestMustNotIncludeHeaders { - type R = MockResponse; - - fn headers_mut(&mut self) -> &mut Headers { &mut self.request_headers } - - fn send(self, _: &Option>) -> Result { - for header in &self.headers_not_expected { - assert!(self.request_headers.get_raw(header).is_none()); - } - - response_for_request_type(self.t) - } -} - struct AssertMustNotIncludeHeadersRequestFactory { headers_not_expected: Vec, body: Vec } impl HttpRequestFactory for AssertMustNotIncludeHeadersRequestFactory { - type R = AssertRequestMustNotIncludeHeaders; + type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { - Ok( - AssertRequestMustNotIncludeHeaders::new( - ResponseType::Text(self.body.clone()), - self.headers_not_expected.clone() - ) - ) + fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + assert!(self.headers_not_expected.len() != 0); + for header in &self.headers_not_expected { + assert!(headers.get_raw(header).is_none()); + } + + Ok(MockRequest::new(ResponseType::Text(self.body.clone()))) } } From 4cb7dfbc59ae664cfa7587b29ae4d5ae7e91b901 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:27:57 -0400 Subject: [PATCH 09/11] Convert remaining factories that don't make use of headers. --- tests/unit/net/http_loader.rs | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 8ac53cebc9f..36e1f120991 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -398,7 +398,7 @@ fn test_request_and_response_data_with_network_messages() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let mut headers = Headers::new(); headers.set(Host { hostname: "foo.bar".to_owned(), port: None }); Ok(MockRequest::new( @@ -473,7 +473,7 @@ fn test_request_and_response_message_from_devtool_without_pipeline_id() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let mut headers = Headers::new(); headers.set(Host { hostname: "foo.bar".to_owned(), port: None }); Ok(MockRequest::new( @@ -504,7 +504,7 @@ fn test_load_when_redirecting_from_a_post_should_rewrite_next_request_as_get() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, url: Url, method: Method) -> Result { + fn create_with_headers(&self, url: Url, method: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { assert_eq!(Method::Post, method); Ok(MockRequest::new(ResponseType::Redirect("http://mozilla.org".to_owned()))) @@ -533,7 +533,7 @@ fn test_load_should_decode_the_response_as_deflate_when_response_headers_have_co impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let mut e = DeflateEncoder::new(Vec::new(), Compression::Default); e.write(b"Yay!").unwrap(); let encoded_content = e.finish().unwrap(); @@ -567,7 +567,7 @@ fn test_load_should_decode_the_response_as_gzip_when_response_headers_have_conte impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let mut e = GzEncoder::new(Vec::new(), Compression::Default); e.write(b"Yay!").unwrap(); let encoded_content = e.finish().unwrap(); @@ -601,7 +601,7 @@ fn test_load_doesnt_send_request_body_on_any_redirect() { impl HttpRequestFactory for Factory { type R = AssertMustHaveBodyRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok( AssertMustHaveBodyRequest::new( @@ -642,7 +642,7 @@ fn test_load_doesnt_add_host_to_sts_list_when_url_is_http_even_if_sts_headers_ar impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set(StrictTransportSecurity::excluding_subdomains(31536000)); @@ -674,7 +674,7 @@ fn test_load_adds_host_to_sts_list_when_url_is_https_and_sts_headers_are_present impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set(StrictTransportSecurity::excluding_subdomains(31536000)); @@ -706,7 +706,7 @@ fn test_load_sets_cookies_in_the_resource_manager_when_it_get_set_cookie_header_ impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set(SetCookie(vec![CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned())])); @@ -846,7 +846,7 @@ fn test_cookie_set_with_httponly_should_not_be_available_using_getcookiesforurl( impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set_raw("set-cookie", vec![b"mozillaIs=theBest; HttpOnly;".to_vec()]); @@ -878,7 +878,7 @@ fn test_when_cookie_received_marked_secure_is_ignored_for_http() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set_raw("set-cookie", vec![b"mozillaIs=theBest; Secure;".to_vec()]); @@ -1061,7 +1061,7 @@ fn test_load_errors_when_there_a_redirect_loop() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok(MockRequest::new(ResponseType::Redirect("http://mozilla.org".to_owned()))) } else if url.domain().unwrap() == "mozilla.org" { @@ -1094,7 +1094,7 @@ fn test_load_errors_when_there_is_too_many_redirects() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok(MockRequest::new(ResponseType::Redirect(format!("{}/1", url.serialize())))) } else { @@ -1125,7 +1125,7 @@ fn test_load_follows_a_redirect() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok(MockRequest::new(ResponseType::Redirect("http://mozilla.org".to_owned()))) } else if url.domain().unwrap() == "mozilla.org" { @@ -1163,7 +1163,7 @@ struct DontConnectFactory; impl HttpRequestFactory for DontConnectFactory { type R = MockRequest; - fn create(&self, url: Url, _: Method) -> Result { + fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { Err(LoadError::Connection(url, "should not have connected".to_owned())) } } @@ -1217,7 +1217,7 @@ fn test_load_errors_when_cancelled() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create(&self, _: Url, _: Method) -> Result { + fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { let mut headers = Headers::new(); headers.set(Host { hostname: "Kaboom!".to_owned(), port: None }); Ok(MockRequest::new( From a761f2bed4379a95fa8c3e32bdee950ca83d5996 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:34:52 -0400 Subject: [PATCH 10/11] Remove unused header manipulation facilities. --- components/net/http_loader.rs | 16 +--------------- tests/unit/net/http_loader.rs | 10 ++-------- 2 files changed, 3 insertions(+), 23 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index e8e0ad682e1..4c40f186cd2 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -249,16 +249,7 @@ impl HttpResponse for WrappedHttpResponse { pub trait HttpRequestFactory { type R: HttpRequest; - fn create(&self, _url: Url, _method: Method) -> Result { - panic!() - } - fn create_with_headers(&self, url: Url, method: Method, headers: Headers) -> Result { - let mut result = self.create(url, method); - if let Ok(ref mut req) = result { - *req.headers_mut() = headers; - } - result - } + fn create_with_headers(&self, url: Url, method: Method, headers: Headers) -> Result; } pub struct NetworkHttpRequestFactory { @@ -300,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>) -> Result; } @@ -311,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>) -> Result { let url = self.request.url.clone(); let mut request_writer = match self.request.start() { diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 36e1f120991..1176d9a92b8 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -142,13 +142,12 @@ enum ResponseType { } struct MockRequest { - headers: Headers, t: ResponseType } impl MockRequest { fn new(t: ResponseType) -> MockRequest { - MockRequest { headers: Headers::new(), t: t } + MockRequest { t: t } } } @@ -175,8 +174,6 @@ fn response_for_request_type(t: ResponseType) -> Result impl HttpRequest for MockRequest { type R = MockResponse; - fn headers_mut(&mut self) -> &mut Headers { &mut self.headers } - fn send(self, _: &Option>) -> Result { response_for_request_type(self.t) } @@ -271,21 +268,18 @@ impl HttpRequestFactory for AssertMustNotIncludeHeadersRequestFactory { struct AssertMustHaveBodyRequest { expected_body: Option>, - headers: Headers, t: ResponseType } impl AssertMustHaveBodyRequest { fn new(t: ResponseType, expected_body: Option>) -> Self { - AssertMustHaveBodyRequest { expected_body: expected_body, headers: Headers::new(), t: t } + AssertMustHaveBodyRequest { expected_body: expected_body, t: t } } } impl HttpRequest for AssertMustHaveBodyRequest { type R = MockResponse; - fn headers_mut(&mut self) -> &mut Headers { &mut self.headers } - fn send(self, body: &Option>) -> Result { assert_eq!(self.expected_body, *body); From 43369fa897711ad19a932b20fb01e3b28eecde90 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Tue, 12 Apr 2016 18:36:16 -0400 Subject: [PATCH 11/11] Rename create_with_headers to create. --- components/net/http_loader.rs | 10 ++++---- tests/unit/net/http_loader.rs | 44 +++++++++++++++++------------------ 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 4c40f186cd2..f27801af52c 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -249,7 +249,7 @@ impl HttpResponse for WrappedHttpResponse { pub trait HttpRequestFactory { type R: HttpRequest; - fn create_with_headers(&self, url: Url, method: Method, headers: Headers) -> Result; + fn create(&self, url: Url, method: Method, headers: Headers) -> Result; } pub struct NetworkHttpRequestFactory { @@ -259,8 +259,8 @@ pub struct NetworkHttpRequestFactory { impl HttpRequestFactory for NetworkHttpRequestFactory { type R = WrappedHttpRequest; - fn create_with_headers(&self, url: Url, method: Method, headers: Headers) - -> Result { + fn create(&self, url: Url, method: Method, headers: Headers) + -> Result { let connection = Request::with_connector(method, url.clone(), &*self.connector); if let Err(HttpError::Ssl(ref error)) = connection { @@ -665,8 +665,8 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, info!("{:?}", data); } - let req = try!(request_factory.create_with_headers(connection_url.clone(), method.clone(), - headers.clone())); + 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())); diff --git a/tests/unit/net/http_loader.rs b/tests/unit/net/http_loader.rs index 1176d9a92b8..e599de3febe 100644 --- a/tests/unit/net/http_loader.rs +++ b/tests/unit/net/http_loader.rs @@ -187,7 +187,7 @@ struct AssertMustHaveHeadersRequestFactory { impl HttpRequestFactory for AssertMustHaveHeadersRequestFactory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + fn create(&self, _: Url, _: Method, headers: Headers) -> Result { assert_eq!(headers, self.expected_headers); Ok(MockRequest::new(ResponseType::Text(self.body.clone()))) } @@ -201,7 +201,7 @@ struct AssertAuthHeaderRequestFactory { impl HttpRequestFactory for AssertAuthHeaderRequestFactory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + fn create(&self, _: Url, _: Method, headers: Headers) -> Result { let request = if headers.has::>() { assert_headers_included(&self.expected_headers, &headers); MockRequest::new(ResponseType::Text(self.body.clone())) @@ -230,7 +230,7 @@ struct AssertMustIncludeHeadersRequestFactory { impl HttpRequestFactory for AssertMustIncludeHeadersRequestFactory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + fn create(&self, _: Url, _: Method, headers: Headers) -> Result { assert_headers_included(&self.expected_headers, &headers); Ok(MockRequest::new(ResponseType::Text(self.body.clone()))) } @@ -256,7 +256,7 @@ struct AssertMustNotIncludeHeadersRequestFactory { impl HttpRequestFactory for AssertMustNotIncludeHeadersRequestFactory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, headers: Headers) -> Result { + fn create(&self, _: Url, _: Method, headers: Headers) -> Result { assert!(self.headers_not_expected.len() != 0); for header in &self.headers_not_expected { assert!(headers.get_raw(header).is_none()); @@ -392,7 +392,7 @@ fn test_request_and_response_data_with_network_messages() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let mut headers = Headers::new(); headers.set(Host { hostname: "foo.bar".to_owned(), port: None }); Ok(MockRequest::new( @@ -467,7 +467,7 @@ fn test_request_and_response_message_from_devtool_without_pipeline_id() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let mut headers = Headers::new(); headers.set(Host { hostname: "foo.bar".to_owned(), port: None }); Ok(MockRequest::new( @@ -498,7 +498,7 @@ fn test_load_when_redirecting_from_a_post_should_rewrite_next_request_as_get() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, url: Url, method: Method, _: Headers) -> Result { + fn create(&self, url: Url, method: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { assert_eq!(Method::Post, method); Ok(MockRequest::new(ResponseType::Redirect("http://mozilla.org".to_owned()))) @@ -527,7 +527,7 @@ fn test_load_should_decode_the_response_as_deflate_when_response_headers_have_co impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let mut e = DeflateEncoder::new(Vec::new(), Compression::Default); e.write(b"Yay!").unwrap(); let encoded_content = e.finish().unwrap(); @@ -561,7 +561,7 @@ fn test_load_should_decode_the_response_as_gzip_when_response_headers_have_conte impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let mut e = GzEncoder::new(Vec::new(), Compression::Default); e.write(b"Yay!").unwrap(); let encoded_content = e.finish().unwrap(); @@ -595,7 +595,7 @@ fn test_load_doesnt_send_request_body_on_any_redirect() { impl HttpRequestFactory for Factory { type R = AssertMustHaveBodyRequest; - fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { + fn create(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok( AssertMustHaveBodyRequest::new( @@ -636,7 +636,7 @@ fn test_load_doesnt_add_host_to_sts_list_when_url_is_http_even_if_sts_headers_ar impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set(StrictTransportSecurity::excluding_subdomains(31536000)); @@ -668,7 +668,7 @@ fn test_load_adds_host_to_sts_list_when_url_is_https_and_sts_headers_are_present impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set(StrictTransportSecurity::excluding_subdomains(31536000)); @@ -700,7 +700,7 @@ fn test_load_sets_cookies_in_the_resource_manager_when_it_get_set_cookie_header_ impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set(SetCookie(vec![CookiePair::new("mozillaIs".to_owned(), "theBest".to_owned())])); @@ -840,7 +840,7 @@ fn test_cookie_set_with_httponly_should_not_be_available_using_getcookiesforurl( impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set_raw("set-cookie", vec![b"mozillaIs=theBest; HttpOnly;".to_vec()]); @@ -872,7 +872,7 @@ fn test_when_cookie_received_marked_secure_is_ignored_for_http() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let content = <[_]>::to_vec("Yay!".as_bytes()); let mut headers = Headers::new(); headers.set_raw("set-cookie", vec![b"mozillaIs=theBest; Secure;".to_vec()]); @@ -1055,7 +1055,7 @@ fn test_load_errors_when_there_a_redirect_loop() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { + fn create(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok(MockRequest::new(ResponseType::Redirect("http://mozilla.org".to_owned()))) } else if url.domain().unwrap() == "mozilla.org" { @@ -1088,7 +1088,7 @@ fn test_load_errors_when_there_is_too_many_redirects() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { + fn create(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok(MockRequest::new(ResponseType::Redirect(format!("{}/1", url.serialize())))) } else { @@ -1119,7 +1119,7 @@ fn test_load_follows_a_redirect() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { + fn create(&self, url: Url, _: Method, _: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { Ok(MockRequest::new(ResponseType::Redirect("http://mozilla.org".to_owned()))) } else if url.domain().unwrap() == "mozilla.org" { @@ -1157,7 +1157,7 @@ struct DontConnectFactory; impl HttpRequestFactory for DontConnectFactory { type R = MockRequest; - fn create_with_headers(&self, url: Url, _: Method, _: Headers) -> Result { + fn create(&self, url: Url, _: Method, _: Headers) -> Result { Err(LoadError::Connection(url, "should not have connected".to_owned())) } } @@ -1211,7 +1211,7 @@ fn test_load_errors_when_cancelled() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, _: Url, _: Method, _: Headers) -> Result { + fn create(&self, _: Url, _: Method, _: Headers) -> Result { let mut headers = Headers::new(); headers.set(Host { hostname: "Kaboom!".to_owned(), port: None }); Ok(MockRequest::new( @@ -1252,7 +1252,7 @@ fn test_redirect_from_x_to_y_provides_y_cookies_from_y() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, url: Url, _: Method, headers: Headers) -> Result { + fn create(&self, url: Url, _: Method, headers: Headers) -> Result { if url.domain().unwrap() == "mozilla.com" { let mut expected_headers_x = Headers::new(); expected_headers_x.set_raw("Cookie".to_owned(), @@ -1324,7 +1324,7 @@ fn test_redirect_from_x_to_x_provides_x_with_cookie_from_first_response() { impl HttpRequestFactory for Factory { type R = MockRequest; - fn create_with_headers(&self, url: Url, _: Method, headers: Headers) -> Result { + fn create(&self, url: Url, _: Method, headers: Headers) -> Result { if url.path().unwrap()[0] == "initial" { let mut initial_answer_headers = Headers::new(); initial_answer_headers.set_raw("set-cookie", vec![b"mozillaIs=theBest; path=/;".to_vec()]);