Auto merge of #8095 - TileHalo:foo, r=Ms2ger

Removed unsafe from 'query_selector_iter' Fixing #8078.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8095)
servo · Oct 20, 2015 · 25d3c2b · 25d3c2b
2 parents 511e3c1 + 89e8a26
commit 25d3c2b
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 55 deletions.
diff --git a/components/script/dom/htmlbuttonelement.rs b/components/script/dom/htmlbuttonelement.rs
@@ -232,13 +232,9 @@ impl<'a> Activatable for &'a HTMLButtonElement {
         if owner.is_none() || elem.click_in_progress() {
             return;
         }
-        // This is safe because we are stopping after finding the first element
-        // and only then performing actions which may modify the DOM tree
-        unsafe {
-            node.query_selector_iter("button[type=submit]".to_owned()).unwrap()
-                .filter_map(HTMLButtonElementCast::to_root)
-                .find(|r| r.r().form_owner() == owner)
-                .map(|s| s.r().synthetic_click_activation(ctrlKey, shiftKey, altKey, metaKey));
-        }
+        node.query_selector_iter("button[type=submit]".to_owned()).unwrap()
+            .filter_map(HTMLButtonElementCast::to_root)
+            .find(|r| r.r().form_owner() == owner)
+            .map(|s| s.r().synthetic_click_activation(ctrlKey, shiftKey, altKey, metaKey));
     }
 }
diff --git a/components/script/dom/htmlinputelement.rs b/components/script/dom/htmlinputelement.rs
@@ -359,12 +359,9 @@ fn broadcast_radio_checked(broadcaster: &HTMLInputElement, group: Option<&Atom>)
     // This function is a workaround for lifetime constraint difficulties.
     fn do_broadcast(doc_node: &Node, broadcaster: &HTMLInputElement,
                         owner: Option<&HTMLFormElement>, group: Option<&Atom>) {
-        // There is no DOM tree manipulation here, so this is safe
-        let iter = unsafe {
-            doc_node.query_selector_iter("input[type=radio]".to_owned()).unwrap()
-                .filter_map(HTMLInputElementCast::to_root)
-                .filter(|r| in_same_group(r.r(), owner, group) && broadcaster != r.r())
-        };
+        let iter = doc_node.query_selector_iter("input[type=radio]".to_owned())
+                .unwrap().filter_map(HTMLInputElementCast::to_root)
+                .filter(|r| in_same_group(r.r(), owner, group) && broadcaster != r.r());
         for ref r in iter {
             if r.r().Checked() {
                 r.r().SetChecked(false);
@@ -714,15 +711,12 @@ impl Activatable for HTMLInputElement {
                     let doc_node = NodeCast::from_ref(doc.r());
                     let group = self.get_radio_group_name();;
 
-                    // Safe since we only manipulate the DOM tree after finding an element
-                    let checked_member = unsafe {
-                        doc_node.query_selector_iter("input[type=radio]".to_owned()).unwrap()
-                                .filter_map(HTMLInputElementCast::to_root)
-                                .find(|r| {
-                                    in_same_group(r.r(), owner.r(), group.as_ref()) &&
-                                    r.r().Checked()
-                                })
-                    };
+                    let checked_member = doc_node.query_selector_iter("input[type=radio]".to_owned()).unwrap()
+                            .filter_map(HTMLInputElementCast::to_root)
+                            .find(|r| {
+                                in_same_group(r.r(), owner.r(), group.as_ref()) &&
+                                r.r().Checked()
+                            });
                     cache.checked_radio = checked_member.r().map(JS::from_ref);
                     cache.checked_changed = self.checked_changed.get();
                     self.SetChecked(true);
@@ -849,43 +843,34 @@ impl Activatable for HTMLInputElement {
         if elem.click_in_progress() {
             return;
         }
-        // This is safe because we are stopping after finding the first element
-        // and only then performing actions which may modify the DOM tree
         let submit_button;
-        unsafe {
-            submit_button = node.query_selector_iter("input[type=submit]".to_owned()).unwrap()
-                .filter_map(HTMLInputElementCast::to_root)
-                .find(|r| r.r().form_owner() == owner);
-        }
+        submit_button = node.query_selector_iter("input[type=submit]".to_owned()).unwrap()
+            .filter_map(HTMLInputElementCast::to_root)
+            .find(|r| r.r().form_owner() == owner);
         match submit_button {
             Some(ref button) => {
                 if button.r().is_instance_activatable() {
                     button.r().synthetic_click_activation(ctrlKey, shiftKey, altKey, metaKey)
                 }
             }
             None => {
-                unsafe {
-                    // Safe because we don't perform any DOM modification
-                    // until we're done with the iterator.
-                    let inputs = node.query_selector_iter("input".to_owned()).unwrap()
-                        .filter_map(HTMLInputElementCast::to_root)
-                        .filter(|input| {
-                            input.r().form_owner() == owner && match &*input.r().Type() {
-                                "text" | "search" | "url" | "tel" |
-                                "email" | "password" | "datetime" |
-                                "date" | "month" | "week" | "time" |
-                                "datetime-local" | "number"
-                                  => true,
-                                _ => false
-                            }
-                        });
+                let inputs = node.query_selector_iter("input".to_owned()).unwrap()
+                    .filter_map(HTMLInputElementCast::to_root)
+                    .filter(|input| {
+                        input.r().form_owner() == owner && match &*input.r().Type() {
+                            "text" | "search" | "url" | "tel" |
+                            "email" | "password" | "datetime" |
+                            "date" | "month" | "week" | "time" |
+                            "datetime-local" | "number"
+                              => true,
+                            _ => false
+                        }
+                    });
 
-                    if inputs.skip(1).next().is_some() {
-                        // lazily test for > 1 submission-blocking inputs
-                        return;
-                    }
+                if inputs.skip(1).next().is_some() {
+                    // lazily test for > 1 submission-blocking inputs
+                    return;
                 }
-
                 form.r().submit(SubmittedFrom::NotFromFormSubmitMethod,
                                 FormSubmitter::FormElement(form.r()));
             }

diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs
@@ -345,8 +345,7 @@ pub struct QuerySelectorIterator {
 }
 
 impl<'a> QuerySelectorIterator {
-    #[allow(unsafe_code)]
-    unsafe fn new(iter: TreeIterator, selectors: Vec<Selector>)
+     fn new(iter: TreeIterator, selectors: Vec<Selector>)
                   -> QuerySelectorIterator {
         QuerySelectorIterator {
             selectors: selectors,
@@ -737,8 +736,7 @@ impl Node {
     /// Get an iterator over all nodes which match a set of selectors
     /// Be careful not to do anything which may manipulate the DOM tree
     /// whilst iterating, otherwise the iterator may be invalidated.
-    #[allow(unsafe_code)]
-    pub unsafe fn query_selector_iter(&self, selectors: DOMString)
+    pub fn query_selector_iter(&self, selectors: DOMString)
                                   -> Fallible<QuerySelectorIterator> {
         // Step 1.
         match parse_author_origin_selector_list_from_str(&selectors) {
@@ -755,7 +753,7 @@ impl Node {
     #[allow(unsafe_code)]
     pub fn query_selector_all(&self, selectors: DOMString) -> Fallible<Root<NodeList>> {
         let window = window_from_node(self);
-        let iter = try!(unsafe { self.query_selector_iter(selectors) });
+        let iter = try!(self.query_selector_iter(selectors));
         Ok(NodeList::new_simple_list(window.r(), iter))
     }