From 29a34dbdb52cb005fd272a67ccb0bcf08363a25b Mon Sep 17 00:00:00 2001 From: Sam Gibson Date: Sat, 18 Jul 2015 10:04:06 +1000 Subject: [PATCH] Resolves code review comments * Lots of rust-isms * Mutable iterator for modifying entries (much better) --- components/net/resource_task.rs | 63 +++++++++++------------------- python/servo/bootstrap_commands.py | 5 ++- tests/unit/net/resource_task.rs | 46 +++++++++++----------- 3 files changed, 48 insertions(+), 66 deletions(-) diff --git a/components/net/resource_task.rs b/components/net/resource_task.rs index 9ee9bb4699c..e4cb9de630e 100644 --- a/components/net/resource_task.rs +++ b/components/net/resource_task.rs @@ -163,17 +163,11 @@ pub fn start_sending_opt(start_chan: LoadConsumer, metadata: Metadata) -> Result } fn preload_hsts_domains() -> Option { - match read_resource_file(&["hsts_preload.json"]) { - Ok(bytes) => { - match from_utf8(&bytes) { - Ok(hsts_preload_content) => { - HSTSList::new_from_preload(hsts_preload_content) - }, - Err(_) => None - } - }, - Err(_) => None - } + read_resource_file(&["hsts_preload.json"]).ok().and_then(|bytes| { + from_utf8(&bytes).ok().and_then(|hsts_preload_content| { + HSTSList::new_from_preload(hsts_preload_content) + }) + }) } /// Create a ResourceTask @@ -246,10 +240,7 @@ pub struct HSTSList { impl HSTSList { pub fn new_from_preload(preload_content: &str) -> Option { - match decode(preload_content) { - Ok(list) => Some(list), - Err(_) => None - } + decode(preload_content).ok() } pub fn is_host_secure(&self, host: &str) -> bool { @@ -286,33 +277,26 @@ impl HSTSList { if !have_domain && !have_subdomain { self.entries.push(entry); } else if !have_subdomain { - self.entries = self.entries.iter().fold(Vec::new(), |mut m, e| { + for e in &mut self.entries { if e.matches_domain(&entry.host) { - // Update the entry if there's an exact domain match. - m.push(entry.clone()); - } else { - // Ignore the new details if it's a subdomain match, or not - // a match at all. Just use the existing entry - m.push(e.clone()); + e.include_subdomains = entry.include_subdomains; + e.max_age = entry.max_age; } - - m - }); + } } } } pub fn secure_load_data(load_data: &LoadData) -> LoadData { - match &*load_data.url.scheme { - "http" => { - let mut secure_load_data = load_data.clone(); - let mut secure_url = load_data.url.clone(); - secure_url.scheme = "https".to_string(); - secure_load_data.url = Url::parse(&secure_url.serialize()).unwrap(); + if &*load_data.url.scheme == "http" { + let mut secure_load_data = load_data.clone(); + let mut secure_url = load_data.url.clone(); + secure_url.scheme = "https".to_string(); + secure_load_data.url = Url::parse(&secure_url.serialize()).unwrap(); - secure_load_data - }, - _ => load_data.clone() + secure_load_data + } else { + load_data.clone() } } @@ -367,10 +351,8 @@ impl ResourceChannelManager { consumer.send(self.resource_manager.cookie_storage.cookies_for_url(&url, source)).unwrap(); } ControlMsg::SetHSTSEntryForHost(host, include_subdomains, max_age) => { - match HSTSEntry::new(host, include_subdomains, max_age) { - Some(entry) => self.resource_manager.add_hsts_entry(entry), - /// Invalid entries (e.g. IP's don't matter) - None => () + if let Some(entry) = HSTSEntry::new(host, include_subdomains, max_age) { + self.resource_manager.add_hsts_entry(entry) } } ControlMsg::Exit => { @@ -428,9 +410,8 @@ impl ResourceManager { } pub fn add_hsts_entry(&mut self, entry: HSTSEntry) { - match self.hsts_list.as_mut() { - Some(list) => list.push(entry), - None => () + if let Some(list) = self.hsts_list.as_mut() { + list.push(entry) } } diff --git a/python/servo/bootstrap_commands.py b/python/servo/bootstrap_commands.py index 3eb501e2377..0e12b786342 100644 --- a/python/servo/bootstrap_commands.py +++ b/python/servo/bootstrap_commands.py @@ -218,10 +218,13 @@ class MachCommands(CommandBase): try: content_base64 = download_bytes("Chromium HSTS preload list", chromium_hsts_url) except urllib2.URLError, e: - print("Unable to download chromium HSTS preload list, are you connected to the internet?") + print("Unable to download chromium HSTS preload list; are you connected to the internet?") sys.exit(1) content_decoded = base64.b64decode(content_base64) + + # The chromium "json" has single line comments in it which, of course, + # are non-standard/non-valid json. Simply strip them out before parsing content_json = re.sub(r'//.*$', '', content_decoded, flags=re.MULTILINE) try: diff --git a/tests/unit/net/resource_task.rs b/tests/unit/net/resource_task.rs index 9f72b4d54ed..55c9baf4107 100644 --- a/tests/unit/net/resource_task.rs +++ b/tests/unit/net/resource_task.rs @@ -2,10 +2,13 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -use net::resource_task::{ - new_resource_task, parse_hostsfile, replace_hosts, HSTSList, HSTSEntry, secure_load_data, - ResourceManager -}; +use net::resource_task::new_resource_task; +use net::resource_task::parse_hostsfile; +use net::resource_task::replace_hosts; +use net::resource_task::HSTSList; +use net::resource_task::HSTSEntry; +use net::resource_task::secure_load_data; +use net::resource_task::ResourceManager; use net_traits::{ControlMsg, LoadData, LoadConsumer}; use net_traits::ProgressMsg; use std::borrow::ToOwned; @@ -83,10 +86,7 @@ fn test_hsts_entry_cant_be_created_with_ipv6_address_as_host() { "2001:0db8:0000:0000:0000:ff00:0042:8329".to_string(), false, None ); - match entry { - Some(_) => panic!("able to create HSTSEntry with IPv6 host"), - None => () - } + assert!(entry.is_none(), "able to create HSTSEntry with IPv6 host"); } #[test] @@ -95,10 +95,7 @@ fn test_hsts_entry_cant_be_created_with_ipv4_address_as_host() { "4.4.4.4".to_string(), false, None ); - match entry { - Some(_) => panic!("able to create HSTSEntry with IPv6 host"), - None => () - } + assert!(entry.is_none(), "able to create HSTSEntry with IPv4 host"); } #[test] @@ -147,6 +144,10 @@ fn test_push_entry_to_hsts_list_should_not_create_duplicate_entry() { assert!(list.entries.len() == 1) } +#[test] +fn test_push_multiple_entrie_to_hsts_list_should_add_them_all() { +} + #[test] fn test_push_entry_to_hsts_list_should_add_an_entry() { let mut list = HSTSList { @@ -154,28 +155,25 @@ fn test_push_entry_to_hsts_list_should_add_an_entry() { }; assert!(!list.is_host_secure("mozilla.org")); + assert!(!list.is_host_secure("bugzilla.org")); list.push(HSTSEntry::new("mozilla.org".to_string(), true, None).unwrap()); + list.push(HSTSEntry::new("bugzilla.org".to_string(), true, None).unwrap()); assert!(list.is_host_secure("mozilla.org")); + assert!(list.is_host_secure("bugzilla.org")); } #[test] fn test_parse_hsts_preload_should_return_none_when_json_invalid() { let mock_preload_content = "derp"; - match HSTSList::new_from_preload(mock_preload_content) { - Some(_) => assert!(false, "preload list should not have parsed"), - None => assert!(true) - } + assert!(HSTSList::new_from_preload(mock_preload_content).is_none(), "invalid preload list should not have parsed") } #[test] fn test_parse_hsts_preload_should_return_none_when_json_contains_no_entries_key() { let mock_preload_content = "{\"nothing\": \"to see here\"}"; - match HSTSList::new_from_preload(mock_preload_content) { - Some(_) => assert!(false, "preload list should not have parsed"), - None => assert!(true) - } + assert!(HSTSList::new_from_preload(mock_preload_content).is_none(), "invalid preload list should not have parsed") } #[test] @@ -189,8 +187,8 @@ fn test_parse_hsts_preload_should_decode_host_and_includes_subdomains() { let hsts_list = HSTSList::new_from_preload(mock_preload_content); let entries = hsts_list.unwrap().entries; - assert!(entries.get(0).unwrap().host == "mozilla.org"); - assert!(entries.get(0).unwrap().include_subdomains == false); + assert!(entries[0].host == "mozilla.org"); + assert!(entries[0].include_subdomains == false); } #[test] @@ -274,7 +272,7 @@ fn test_secure_load_data_does_not_affect_non_http_schemas() { let load_data = LoadData::new(Url::parse("file://mozilla.org").unwrap(), None); let secure = secure_load_data(&load_data); - assert!(&secure.url.scheme == "file"); + assert_eq!(&secure.url.scheme, "file"); } #[test] @@ -282,7 +280,7 @@ fn test_secure_load_data_forces_an_http_host_in_list_to_https() { let load_data = LoadData::new(Url::parse("http://mozilla.org").unwrap(), None); let secure = secure_load_data(&load_data); - assert!(&secure.url.scheme == "https"); + assert_eq!(&secure.url.scheme, "https"); } #[test]