Add is_mathml_annotation_xml_integration_point method #186

GuillaumeGomez · 2016-01-04T22:42:08Z

SimonSapin · 2016-01-05T01:09:10Z

src/rcdom.rs

+        for it in 0..attrs.len() {
+            match attrs[it].name {
+                qualname!(html, "encoding") => {
+                    return new_node(Element(name, AnnotationXml(attrs[it].value.to_lowercase() == "text/html"), attrs));


Use .eq_ignore_ascii_case("text/html") to save a memory allocation.

SimonSapin · 2016-01-05T01:09:13Z

src/rcdom.rs

@@ -45,6 +45,7 @@ pub enum ElementEnum {
    /// A template element and its template contents.
    /// https://html.spec.whatwg.org/multipage/#template-contents
    Template(Handle),
+    AnnotationXml(bool),


What does it mean for an element to "be" this variant? Is it exclusive with other variants? What does the boolean mean? Consider using a struct-like variant in order to name the boolean. Please add a doc-comment with a link to the relevant section of spec.

I didn't add comments yet, it was just supposed to be a first shot to get help. on some details before finishing it. I should have precised, my bad.

nox · 2016-01-05T01:12:02Z

src/tree_builder/interface.rs

+
+    // Returns true if the adjusted current node is an HTML integration point
+    // and the token is a start tag
+    fn is_mathml_annotation_xml_integration_point(&self) -> bool;


This should take a Self::Handle and that's the thing on which you check whether it is an integration point or not.

nox · 2016-01-05T01:12:28Z

src/rcdom.rs

@@ -303,6 +312,13 @@ impl TreeSink for RcDom {
            panic!("not a script element!");
        }
    }
+
+    fn is_mathml_annotation_xml_integration_point(&self) -> bool {
+        match (**self.document).borrow().node {


The thing you need to check is the given Handle that needs to be passed to the method.

GuillaumeGomez · 2016-01-05T22:05:39Z

Ok, I updated the current code @nox.

nox · 2016-01-06T00:48:54Z

src/rcdom.rs

+    fn is_mathml_annotation_xml_integration_point(&self, handle: &Self::Handle) -> bool {
+        match (**handle).borrow().node {
+            Element(_, AnnotationXml(ret), _) => ret,
+            _ => false,


This should be unreachable.

nox · 2016-01-06T00:50:22Z

src/rcdom.rs

+                }
+                _ => continue,
+            }
+        }


This should be in the match expression defining info below, for qualname!(mathml, "annotation-xml") elements.

GuillaumeGomez · 2016-01-10T13:26:58Z

Anything else to add/update?

nox · 2016-01-10T15:03:38Z

src/tree_builder/actions.rs

@@ -901,11 +901,15 @@ impl<Handle, Sink> TreeBuilderActions<Handle>

        if let qualname!(mathml, "annotation-xml") = name {
            if let TagToken(Tag { kind: StartTag, name: atom!("svg"), .. }) = *token {


This condition should go away. It has nothing to do here.

I was wrong about this.

nox · 2016-01-10T15:04:05Z

src/tree_builder/interface.rs

+
+    // Returns true if the adjusted current node is an HTML integration point
+    // and the token is a start tag
+    fn is_mathml_annotation_xml_integration_point(&self, handle: &Self::Handle) -> bool {


This should take Self::Handle, and you should clone handles when passing them.


        Add is_mathml_annotation_xml_integration_point method

nox · 2016-01-11T11:42:02Z

@bors-servo r+

bors-servo · 2016-01-11T11:42:04Z

📌 Commit 3de1587 has been approved by nox

bors-servo · 2016-01-11T11:42:04Z

⌛ Testing commit 3de1587 with merge c3dea24...


        Auto merge of #186 - GuillaumeGomez:master, r=nox

@nox

Add is_mathml_annotation_xml_integration_point method r? @nox  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/186)

bors-servo · 2016-01-11T11:47:11Z

☀️ Test successful - travis

SimonSapin reviewed Jan 5, 2016
View changes

nox reviewed Jan 5, 2016
View changes

nox reviewed Jan 6, 2016
View changes

src/rcdom.rs Outdated

}

_ => continue,

}

}

This comment has been minimized.

Sign in to view

nox Jan 6, 2016
Member

This should be in the match expression defining info below, for qualname!(mathml, "annotation-xml") elements.

nox reviewed Jan 10, 2016
View changes

Add is_mathml_annotation_xml_integration_point method

Loading status checks…

3de1587

nox self-assigned this Jan 11, 2016

bors-servo merged commit 3de1587 into servo:master Jan 11, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / html5ever

Add is_mathml_annotation_xml_integration_point method #186

Add is_mathml_annotation_xml_integration_point method #186

GuillaumeGomez commented Jan 4, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jan 5, 2016

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jan 10, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Jan 11, 2016

bors-servo commented Jan 11, 2016

bors-servo commented Jan 11, 2016

bors-servo commented Jan 11, 2016

		@@ -901,11 +901,15 @@ impl<Handle, Sink> TreeBuilderActions<Handle>

		if let qualname!(mathml, "annotation-xml") = name {
		if let TagToken(Tag { kind: StartTag, name: atom!("svg"), .. }) = *token {

servo / html5ever

Join GitHub today

Add is_mathml_annotation_xml_integration_point method #186

Add is_mathml_annotation_xml_integration_point method #186

Conversation

GuillaumeGomez commented Jan 4, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jan 5, 2016

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez commented Jan 10, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented Jan 11, 2016

bors-servo commented Jan 11, 2016

bors-servo commented Jan 11, 2016

bors-servo commented Jan 11, 2016