core: Remove extra Option/Results from XmlNode getters

This removes some special cases and simplifies the code.

core/src/avm1/globals/xml.rs

@@ -207,7 +207,7 @@ pub fn xmlnode_remove_node<'gc>(
     _args: &[Value<'gc>],
 ) -> Result<Value<'gc>, Error<'gc>> {
     if let Some(node) = this.as_xml_node() {
-        if let Ok(Some(mut parent)) = node.parent() {
+        if let Some(mut parent) = node.parent() {
             if let Err(e) = parent.remove_child(activation.context.gc_context, node) {
                 avm_warn!(activation, "Error in XML.removeNode: {}", e);
             }
@@ -323,26 +323,25 @@ pub fn xmlnode_child_nodes<'gc>(
             activation.context.gc_context,
             Some(activation.context.avm1.prototypes.array),
         );
-        if let Some(children) = node.children() {
-            let mut compatible_nodes = 0;
-            for mut child in children {
-                if !is_as2_compatible(child) {
-                    continue;
-                }
 
-                array.set_array_element(
+        let mut compatible_nodes = 0;
-                    compatible_nodes as usize,
+        for mut child in node.children() {
-                    child
+            if !is_as2_compatible(child) {
-                        .script_object(
+                continue;
-                            activation.context.gc_context,
-                            Some(activation.context.avm1.prototypes.xml_node),
-                        )
-                        .into(),
-                    activation.context.gc_context,
-                );
-
-                compatible_nodes += 1;
             }
+
+            array.set_array_element(
+                compatible_nodes as usize,
+                child
+                    .script_object(
+                        activation.context.gc_context,
+                        Some(activation.context.avm1.prototypes.xml_node),
+                    )
+                    .into(),
+                activation.context.gc_context,
+            );
+
+            compatible_nodes += 1;
         }
 
         return Ok(array.into());
@@ -357,27 +356,26 @@ pub fn xmlnode_first_child<'gc>(
     _args: &[Value<'gc>],
 ) -> Result<Value<'gc>, Error<'gc>> {
     if let Some(node) = this.as_xml_node() {
-        if let Some(mut children) = node.children() {
+        let mut children = node.children();
-            let mut next = children.next();
+        let mut next = children.next();
-            while let Some(my_next) = next {
+        while let Some(my_next) = next {
-                if is_as2_compatible(my_next) {
+            if is_as2_compatible(my_next) {
-                    break;
+                break;
-                }
-
-                next = my_next.next_sibling().unwrap_or(None);
             }
 
-            return Ok(next
+            next = my_next.next_sibling();
-                .map(|mut child| {
-                    child
-                        .script_object(
-                            activation.context.gc_context,
-                            Some(activation.context.avm1.prototypes.xml_node),
-                        )
-                        .into()
-                })
-                .unwrap_or_else(|| Value::Null));
         }
+
+        return Ok(next
+            .map(|mut child| {
+                child
+                    .script_object(
+                        activation.context.gc_context,
+                        Some(activation.context.avm1.prototypes.xml_node),
+                    )
+                    .into()
+            })
+            .unwrap_or_else(|| Value::Null));
     }
 
     Ok(Value::Undefined)
@@ -389,26 +387,25 @@ pub fn xmlnode_last_child<'gc>(
     _args: &[Value<'gc>],
 ) -> Result<Value<'gc>, Error<'gc>> {
     if let Some(node) = this.as_xml_node() {
-        if let Some(mut children) = node.children() {
+        let mut children = node.children();
-            let mut prev = children.next_back();
+        let mut prev = children.next_back();
-            while let Some(my_prev) = prev {
+        while let Some(my_prev) = prev {
-                if is_as2_compatible(my_prev) {
+            if is_as2_compatible(my_prev) {
-                    break;
+                break;
-                }
-
-                prev = my_prev.prev_sibling().unwrap_or(None);
             }
-            return Ok(prev
+
-                .map(|mut child| {
+            prev = my_prev.prev_sibling();
-                    child
-                        .script_object(
-                            activation.context.gc_context,
-                            Some(activation.context.avm1.prototypes.xml_node),
-                        )
-                        .into()
-                })
-                .unwrap_or_else(|| Value::Null));
         }
+        return Ok(prev
+            .map(|mut child| {
+                child
+                    .script_object(
+                        activation.context.gc_context,
+                        Some(activation.context.avm1.prototypes.xml_node),
+                    )
+                    .into()
+            })
+            .unwrap_or_else(|| Value::Null));
     }
 
     Ok(Value::Undefined)
@@ -422,7 +419,6 @@ pub fn xmlnode_parent_node<'gc>(
     if let Some(node) = this.as_xml_node() {
         return Ok(node
             .parent()
-            .unwrap_or(None)
             .map(|mut parent| {
                 parent
                     .script_object(
@@ -443,13 +439,13 @@ pub fn xmlnode_previous_sibling<'gc>(
     _args: &[Value<'gc>],
 ) -> Result<Value<'gc>, Error<'gc>> {
     if let Some(node) = this.as_xml_node() {
-        let mut prev = node.prev_sibling().unwrap_or(None);
+        let mut prev = node.prev_sibling();
         while let Some(my_prev) = prev {
             if is_as2_compatible(my_prev) {
                 break;
             }
 
-            prev = my_prev.prev_sibling().unwrap_or(None);
+            prev = my_prev.prev_sibling();
         }
 
         return Ok(prev
@@ -472,13 +468,13 @@ pub fn xmlnode_next_sibling<'gc>(
     _args: &[Value<'gc>],
 ) -> Result<Value<'gc>, Error<'gc>> {
     if let Some(node) = this.as_xml_node() {
-        let mut next = node.next_sibling().unwrap_or(None);
+        let mut next = node.next_sibling();
         while let Some(my_next) = next {
             if is_as2_compatible(my_next) {
                 break;
             }
 
-            next = my_next.next_sibling().unwrap_or(None);
+            next = my_next.next_sibling();
         }
 
         return Ok(next
@@ -891,17 +887,15 @@ pub fn xml_parse_xml<'gc>(
                 "".into()
             };
 
-        if let Some(children) = node.children() {
+        for child in node.children().rev() {
-            for child in children.rev() {
+            let result = node.remove_child(activation.context.gc_context, child);
-                let result = node.remove_child(activation.context.gc_context, child);
+            if let Err(e) = result {
-                if let Err(e) = result {
+                avm_warn!(
-                    avm_warn!(
+                    activation,
-                        activation,
+                    "XML.parseXML: Error removing node contents: {}",
-                        "XML.parseXML: Error removing node contents: {}",
+                    e
-                        e
+                );
-                    );
+                return Ok(Value::Undefined);
-                    return Ok(Value::Undefined);
-                }
             }
         }
 

core/src/html/text_format.rs

@@ -1600,7 +1600,7 @@ impl FormatSpans {
         self.text = "".to_string();
         self.spans = vec![];
 
-        for step in tree.as_node().walk().unwrap() {
+        for step in tree.as_node().walk() {
             match step {
                 Step::In(node)
                     if node

core/src/xml/iterators.rs

@@ -142,18 +142,15 @@ impl<'gc> Iterator for WalkIter<'gc> {
     }
 }
 
-/// Iterator that yields indirect descendents of an XML node.
+/// Iterator that yields the ancestors of an XML node.
 pub struct AnscIter<'gc> {
     next: Option<XmlNode<'gc>>,
 }
 
 impl<'gc> AnscIter<'gc> {
-    /// Construct a new `AnscIter` that lists the parents of an XML node.
+    /// Construct a new `AnscIter` that lists the parents of an XML node (including itself).
-    ///
+    pub fn for_node(node: XmlNode<'gc>) -> Self {
-    /// This function should be called with the parent of the node being
+        Self { next: Some(node) }
-    /// iterated.
-    pub fn for_node(next: Option<XmlNode<'gc>>) -> Self {
-        Self { next }
     }
 }
 
@@ -164,10 +161,7 @@ impl<'gc> Iterator for AnscIter<'gc> {
         let parent = self.next;
 
         if let Some(parent) = parent {
-            match parent.parent() {
+            self.next = parent.parent();
-                Ok(gp) => self.next = gp,
-                _ => self.next = None,
-            }
         }
 
         parent

core/src/xml/tests.rs

@@ -12,16 +12,13 @@ fn parse_single_element() {
         xml.as_node()
             .replace_with_str(mc, "<test></test>", true, false)
             .expect("Parsed document");
-        let mut roots = xml
+        let mut roots = xml.as_node().children();
-            .as_node()
-            .children()
-            .expect("Parsed document should be capable of having child nodes");
 
         let root = roots.next().expect("Parsed document should have a root");
         assert_eq!(root.node_type(), xml::ELEMENT_NODE);
         assert_eq!(root.tag_name(), Some(XmlName::from_str("test")));
 
-        let mut root_children = root.children().unwrap();
+        let mut root_children = root.children();
         assert!(root_children.next().is_none());
 
         assert!(roots.next().is_none());
@@ -42,10 +39,7 @@ fn double_ended_children() {
             )
             .expect("Parsed document");
 
-        let mut roots = xml
+        let mut roots = xml.as_node().children();
-            .as_node()
-            .children()
-            .expect("Parsed document should be capable of having child nodes");
 
         let root = roots.next().expect("Should have first root");
         assert_eq!(root.node_type(), xml::ELEMENT_NODE);
@@ -87,10 +81,7 @@ fn walk() {
             )
             .expect("Parsed document");
 
-        let mut roots = xml
+        let mut roots = xml.as_node().walk();
-            .as_node()
-            .walk()
-            .expect("Parsed document should be capable of having child nodes");
 
         let root = roots.next().expect("Should have first root");
         assert!(root.stepped_in());
@@ -211,36 +202,21 @@ fn ignore_white() {
             )
             .expect("Parsed document");
 
-        let mut root = xml
+        let mut root = xml.as_node().children();
-            .as_node()
-            .children()
-            .expect("Parsed document should be capable of having child nodes");
 
         let mut node = root.next().expect("Should have root");
         assert_eq!(node.node_type(), xml::ELEMENT_NODE);
         assert_eq!(node.tag_name(), Some(XmlName::from_str("test")));
 
-        node = node
+        node = node.children().next().expect("Should have children");
-            .children()
-            .expect("Should have children")
-            .next()
-            .expect("Should have children");
         assert_eq!(node.node_type(), xml::ELEMENT_NODE);
         assert_eq!(node.tag_name(), Some(XmlName::from_str("test2")));
 
-        node = node
+        node = node.children().next().expect("Should have children");
-            .children()
-            .expect("Should have children")
-            .next()
-            .expect("Should have children");
         assert_eq!(node.node_type(), xml::ELEMENT_NODE);
         assert_eq!(node.tag_name(), Some(XmlName::from_str("test3")));
 
-        node = node
+        node = node.children().next().expect("Should have text");
-            .children()
-            .expect("Should have children")
-            .next()
-            .expect("Should have text");
         assert_eq!(node.node_type(), xml::TEXT_NODE);
         assert_eq!(node.node_value(), Some(" foo ".to_string()));
 

core/src/xml/tree.rs

@@ -488,28 +488,24 @@ impl<'gc> XmlNode<'gc> {
     }
 
     /// Get the parent, if this node has one.
-    ///
+    pub fn parent(self) -> Option<XmlNode<'gc>> {
-    /// If the node cannot have a parent, then this function yields Err.
-    pub fn parent(self) -> Result<Option<XmlNode<'gc>>, Error> {
         match *self.0.read() {
-            XmlNodeData::DocumentRoot { .. } => Err(Error::RootCantHaveParent),
+            XmlNodeData::DocumentRoot { .. } => None,
-            XmlNodeData::Element { parent, .. } => Ok(parent),
+            XmlNodeData::Element { parent, .. } => parent,
-            XmlNodeData::Text { parent, .. } => Ok(parent),
+            XmlNodeData::Text { parent, .. } => parent,
-            XmlNodeData::Comment { parent, .. } => Ok(parent),
+            XmlNodeData::Comment { parent, .. } => parent,
-            XmlNodeData::DocType { parent, .. } => Ok(parent),
+            XmlNodeData::DocType { parent, .. } => parent,
         }
     }
 
     /// Get the previous sibling, if this node has one.
-    ///
+    pub fn prev_sibling(self) -> Option<XmlNode<'gc>> {
-    /// If the node cannot have siblings, then this function yields Err.
-    pub fn prev_sibling(self) -> Result<Option<XmlNode<'gc>>, Error> {
         match *self.0.read() {
-            XmlNodeData::DocumentRoot { .. } => Err(Error::RootCantHaveSiblings),
+            XmlNodeData::DocumentRoot { .. } => None,
-            XmlNodeData::Element { prev_sibling, .. } => Ok(prev_sibling),
+            XmlNodeData::Element { prev_sibling, .. } => prev_sibling,
-            XmlNodeData::Text { prev_sibling, .. } => Ok(prev_sibling),
+            XmlNodeData::Text { prev_sibling, .. } => prev_sibling,
-            XmlNodeData::Comment { prev_sibling, .. } => Ok(prev_sibling),
+            XmlNodeData::Comment { prev_sibling, .. } => prev_sibling,
-            XmlNodeData::DocType { prev_sibling, .. } => Ok(prev_sibling),
+            XmlNodeData::DocType { prev_sibling, .. } => prev_sibling,
         }
     }
 
@@ -531,15 +527,13 @@ impl<'gc> XmlNode<'gc> {
     }
 
     /// Get the next sibling, if this node has one.
-    ///
+    pub fn next_sibling(self) -> Option<XmlNode<'gc>> {
-    /// If the node cannot have siblings, then this function yields Err.
-    pub fn next_sibling(self) -> Result<Option<XmlNode<'gc>>, Error> {
         match *self.0.read() {
-            XmlNodeData::DocumentRoot { .. } => Err(Error::RootCantHaveSiblings),
+            XmlNodeData::DocumentRoot { .. } => None,
-            XmlNodeData::Element { next_sibling, .. } => Ok(next_sibling),
+            XmlNodeData::Element { next_sibling, .. } => next_sibling,
-            XmlNodeData::Text { next_sibling, .. } => Ok(next_sibling),
+            XmlNodeData::Text { next_sibling, .. } => next_sibling,
-            XmlNodeData::Comment { next_sibling, .. } => Ok(next_sibling),
+            XmlNodeData::Comment { next_sibling, .. } => next_sibling,
-            XmlNodeData::DocType { next_sibling, .. } => Ok(next_sibling),
+            XmlNodeData::DocType { next_sibling, .. } => next_sibling,
         }
     }
 
@@ -568,8 +562,8 @@ impl<'gc> XmlNode<'gc> {
     /// This is the opposite of `adopt_siblings` - the former adds a node to a
     /// new sibling list, and this removes it from the current one.
     fn disown_siblings(&mut self, mc: MutationContext<'gc, '_>) -> Result<(), Error> {
-        let old_prev = self.prev_sibling()?;
+        let old_prev = self.prev_sibling();
-        let old_next = self.next_sibling()?;
+        let old_next = self.next_sibling();
 
         if let Some(mut prev) = old_prev {
             prev.set_next_sibling(mc, old_next)?;
@@ -661,16 +655,13 @@ impl<'gc> XmlNode<'gc> {
         position: usize,
         child: XmlNode<'gc>,
     ) -> Result<(), Error> {
-        if GcCell::ptr_eq(self.0, child.0) {
+        let is_cyclic = self
+            .ancestors()
+            .any(|ancestor| GcCell::ptr_eq(ancestor.0, child.0));
+        if is_cyclic {
             return Err(Error::CannotInsertIntoSelf);
         }
 
-        if let Some(mut it) = self.ancestors() {
-            if it.find(|ancestor| GcCell::ptr_eq(ancestor.0, child.0)).is_some() {
-                return Err(Error::CannotInsertIntoSelf);
-            }
-        }
-
         match &mut *self.0.write(mc) {
             XmlNodeData::Element {
                 ref mut children, ..
@@ -774,22 +765,14 @@ impl<'gc> XmlNode<'gc> {
     /// This function yields None if the node cannot accept children or if the
     /// child node is not a child of this node.
     pub fn child_position(self, child: XmlNode<'gc>) -> Option<usize> {
-        if let Some(children) = self.children() {
+        self.children()
-            for (i, other_child) in children.enumerate() {
+            .position(|other| GcCell::ptr_eq(child.0, other.0))
-                if GcCell::ptr_eq(child.0, other_child.0) {
-                    return Some(i);
-                }
-            }
-        }
-
-        None
     }
 
     /// Checks if `child` is a direct descendant of `self`.
     pub fn has_child(self, child: XmlNode<'gc>) -> bool {
         child
             .parent()
-            .unwrap_or(None)
             .filter(|p| GcCell::ptr_eq(self.0, p.0))
             .is_some()
     }
@@ -818,15 +801,8 @@ impl<'gc> XmlNode<'gc> {
     }
 
     /// Returns an iterator that yields child nodes.
-    ///
+    pub fn children(self) -> impl DoubleEndedIterator<Item = XmlNode<'gc>> {
-    /// Yields None if this node cannot accept children.
+        xml::iterators::ChildIter::for_node(self)
-    pub fn children(self) -> Option<impl DoubleEndedIterator<Item = XmlNode<'gc>>> {
-        match &*self.0.read() {
-            XmlNodeData::Element { .. } | XmlNodeData::DocumentRoot { .. } => {
-                Some(xml::iterators::ChildIter::for_node(self))
-            }
-            _ => None,
-        }
     }
 
     /// Returns an iterator that walks the XML tree.
@@ -834,30 +810,13 @@ impl<'gc> XmlNode<'gc> {
     /// Walking is similar to using `descendents`, but the ends of parent nodes
     /// are explicitly marked with `Step::Out`, while nodes that may have
     /// children are marked with `Step::In`.
-    ///
+    pub fn walk(self) -> impl Iterator<Item = Step<'gc>> {
-    /// Yields None if this node cannot accept children.
+        xml::iterators::WalkIter::for_node(self)
-    pub fn walk(self) -> Option<impl Iterator<Item = Step<'gc>>> {
-        match &*self.0.read() {
-            XmlNodeData::Element { .. } | XmlNodeData::DocumentRoot { .. } => {
-                Some(xml::iterators::WalkIter::for_node(self))
-            }
-            _ => None,
-        }
     }
 
-    /// Returns an iterator that yields ancestor nodes.
+    /// Returns an iterator that yields ancestor nodes (including itself).
-    ///
+    pub fn ancestors(self) -> impl Iterator<Item = XmlNode<'gc>> {
-    /// Yields None if this node does not have a parent.
+        xml::iterators::AnscIter::for_node(self)
-    pub fn ancestors(self) -> Option<impl Iterator<Item = XmlNode<'gc>>> {
-        let parent = match *self.0.read() {
-            XmlNodeData::DocumentRoot { .. } => return None,
-            XmlNodeData::Element { parent, .. } => parent,
-            XmlNodeData::Text { parent, .. } => parent,
-            XmlNodeData::Comment { parent, .. } => parent,
-            XmlNodeData::DocType { parent, .. } => parent,
-        };
-
-        Some(xml::iterators::AnscIter::for_node(parent))
     }
 
     /// Get the already-instantiated script object from the current node.
@@ -1046,12 +1005,10 @@ impl<'gc> XmlNode<'gc> {
         document.link_root_node(gc_context, clone);
 
         if deep {
-            if let Some(children) = self.children() {
+            for (position, child) in self.children().enumerate() {
-                for (position, child) in children.enumerate() {
+                clone
-                    clone
+                    .insert_child(gc_context, position, child.duplicate(gc_context, deep))
-                        .insert_child(gc_context, position, child.duplicate(gc_context, deep))
+                    .expect("If I can see my children then my clone should accept children");
-                        .expect("If I can see my children then my clone should accept children");
-                }
             }
         }
 
@@ -1134,7 +1091,7 @@ impl<'gc> XmlNode<'gc> {
             return Some(url);
         }
 
-        if let Ok(Some(parent)) = self.parent() {
+        if let Some(parent) = self.parent() {
             parent.lookup_uri_for_namespace(namespace)
         } else {
             None
@@ -1179,7 +1136,7 @@ impl<'gc> XmlNode<'gc> {
     pub fn lookup_namespace_for_uri(self, uri: &str) -> Option<String> {
         if let Some(xname) = self.value_attribute(uri, Some("xmlns")) {
             Some(xname.local_name().to_string())
-        } else if let Ok(Some(parent)) = self.parent() {
+        } else if let Some(parent) = self.parent() {
             parent.lookup_namespace_for_uri(uri)
         } else {
             None
@@ -1218,15 +1175,7 @@ impl<'gc> XmlNode<'gc> {
         W: Write,
         F: FnMut(XmlNode<'gc>) -> bool,
     {
-        let mut children = Vec::new();
+        let children: Vec<_> = self.children().filter(|child| filter(*child)).collect();
-        if let Some(my_children) = self.children() {
-            for child in my_children {
-                if filter(child) {
-                    children.push(child)
-                }
-            }
-        }
-
         let children_len = children.len();
 
         match &*self.0.read() {