Resolves code review comments

* Lots of rust-isms
* Mutable iterator for modifying entries (much better)

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<HSTSList> {
-    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<HSTSList> {
-        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();
-
-            secure_load_data
-        },
-        _ => load_data.clone()
+    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
+    } 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)
         }
     }
 

python/servo/bootstrap_commands.py

@@ -218,10 +218,13 @@ def bootstrap_hsts_preload(self, force=False):
         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:

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,35 +144,36 @@ 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 {
         entries: Vec::new()
     };
 
     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,15 +272,15 @@ 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]
 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]