Added current_line variable to Tokenizer and test to check that curre… #240

karenher · 2016-12-01T22:33:50Z

…nt_line is correctly updated

This change is


        Added current_line variable to Tokenizer and test to check that curre…

…nt_line is correctly updated


        Added in missing parameter

jdm

Great work! I've noted some code style nits, as well as some changes that should improve correctness.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

@@ -200,6 +203,7 @@ impl<Sink: TokenSink> Tokenizer<Sink> {
            temp_buf: StrTendril::new(),
            state_profile: BTreeMap::new(),
            time_in_sink: 0,
+	    current_line: 1,


nit: replace the tabs in this line with spaces.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+
+
+
+


nit: remove these new blank lines.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+
+    pub use super::interface::{Token, DoctypeToken, TagToken, CommentToken};
+    pub use super::interface::{CharacterTokens, NullCharacterToken, EOFToken, ParseError};
+    pub use super::interface::{Doctype, Attribute, TagKind, StartTag, EndTag, Tag};


The pub in these lines isn't necessary.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+    struct TokenMatch {
+        tokens: Vec<Token>,
+        current_str: StrTendril,
+        exact_errors: bool,


We don't need the tokens or exact_errors members for this test.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+    // TokenMatch implements the TokenSink trait. It is used for testing to see 
+    // if current_line is being updated when process_token is called. The lines
+    // vector is a collection of the line numbers that each token is on.
+    struct TokenMatch {


Let's call this LinesMatch instead, since that's what it's testing.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+    #[test]
+    fn check_lines() {
+
+        let opts = TokenizerOpts{exact_errors: false, discard_bom: true, profile: false,


nit: space before and after {

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+    fn check_lines() {
+
+        let opts = TokenizerOpts{exact_errors: false, discard_bom: true, profile: false,
+            initial_state: None, last_start_tag_name: None,};


nit: move }; to the next line, like:

let opts = TokenizerOpts { ..., ..., };

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

@@ -270,6 +274,7 @@ impl<Sink: TokenSink> Tokenizer<Sink> {
        if c == '\r' {
            self.ignore_lf = true;
            c = '\n';
+	    self.current_line += 1;


Let's make a separate block after this instead:

if c == '\r' { self.ignore_lf = true; c = '\n'; } if c == '\n' { self.current_line += 1; }

This means that input that only contains \n tokens will correctly count lines, and code that contains \r\n tokens (which is what the ignore_lf checks are for) will also count lines correctly.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+            initial_state: None, last_start_tag_name: None,};
+
+        let vector = vec![StrTendril::from("<a>\r"), StrTendril::from("<b>\r"),
+            StrTendril::from("</b>\r"), StrTendril::from("</a>\r")];


We should run this test twice - once using \n separators, and once using \r\n.

jdm · 2016-12-09T19:55:02Z

src/tokenizer/mod.rs

+
+        let vector = vec![StrTendril::from("<a>\r"), StrTendril::from("<b>\r"),
+            StrTendril::from("</b>\r"), StrTendril::from("</a>\r")];
+        let expected = vec![1, 2, 2, 3, 3, 4, 4, 5, 5];


We'll need to update this to contain pairs of tokens and the corresponding line number.


        Made test check tuples of tokens and line numbers and changed other s…

…tyle issues


        Added set_current_line function and test to treebuilder

jdm

I've mostly requested small changes; we'll just need to make sure that the tests are verifying what we want, and that we're processing \n characters as expected.

jdm · 2016-12-22T20:49:01Z

benches/tokenizer.rs

@@ -29,7 +29,7 @@ struct Sink;
 impl TokenSink for Sink {
    type Handle = ();

-    fn process_token(&mut self, token: Token) -> TokenSinkResult<()> {
+    fn process_token(&mut self, token: Token, line_number: u64) -> TokenSinkResult<()> {


Let's use _line_number here and elsewhere to avoid warnings about unused variables in code that never makes use of it.

jdm · 2016-12-22T20:49:01Z

src/rcdom.rs

@@ -161,6 +161,9 @@ pub struct RcDom {

    /// The document's quirks mode.
    pub quirks_mode: QuirksMode,
+
+    /// The current line being parsed
+    pub current_line: u64,


Let's remove this, since no RcDom code actually uses it.

jdm · 2016-12-22T20:49:01Z

src/rcdom.rs

@@ -326,6 +329,10 @@ impl TreeSink for RcDom {
            _ => unreachable!(),
        }
    }
+
+    fn set_current_line(&mut self, line_number: u64) {
+        self.current_line = line_number;


This can be an empty method instead (and use _line_number to avoid warnings).

jdm · 2016-12-22T20:49:01Z

src/tokenizer/mod.rs

+
+        fn push(&mut self, token: Token, line_number: u64) {
+            self.finish_str();
+            // self.tokens.push(token);


Let's remove this.

jdm · 2016-12-22T20:49:01Z

src/tokenizer/mod.rs

+
+    use super::buffer_queue::{BufferQueue};
+    use std::mem::replace;
+    use std::borrow::Cow::{self, Borrowed};


We don't use Cow anywhere, so this import can be removed.

jdm · 2016-12-22T20:49:01Z

src/tree_builder/mod.rs

+    pub struct LineCountingDOM {
+        pub line_vec: Vec<(QualName, u64)>,
+        pub rcdom: RcDom,
+    }


Let's store the current line in this structure, rather than RcDom.

jdm · 2016-12-22T20:49:01Z

src/tree_builder/mod.rs

+        }
+    }
+
+    impl Default for LineCountingDOM {


We don't need this implementation; we can write out the initializer by hand instead of using LineCountingDOM::default().

jdm · 2016-12-22T20:49:01Z

src/tree_builder/mod.rs

+        resultTok.process(StrTendril::from("<a>"));
+        resultTok.process(StrTendril::from("</a>"));
+        resultTok.process(StrTendril::from("<b>"));
+        resultTok.process(StrTendril::from("</b>"));


We're going to want some newlines in this test, or it's not actually doing much good ;)

jdm · 2016-12-22T20:49:01Z

src/tree_builder/mod.rs

+                            (qualname!(html, "head"), 1),
+                            (qualname!(html, "body"), 1),
+                            (qualname!(html, "a"), 1),
+                            (qualname!(html, "b"), 1)];


Our expected output will want to ensure that at least some elements are associated with different lines.

jdm · 2016-12-22T20:49:01Z

src/tree_builder/mod.rs

+                            (qualname!(html, "body"), 1),
+                            (qualname!(html, "a"), 1),
+                            (qualname!(html, "b"), 1)];
+        let result = actual.line_vec.clone();


Why is this necessary?


        Fixed style issues and test. Added '\n' to pop_except_from function c…

…all in step function to account to make sure current_line is updated.


        Auto merge of #243 - jdm:lines, r=jdm

Track current line when parsing Rebased from #240.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/243)

jdm · 2016-12-28T17:41:25Z

Everything here looked good to me, so I squashed the commits in #243 and merged them there. Thanks for working on this!

karenher added 2 commits Dec 1, 2016

Added current_line variable to Tokenizer and test to check that curre…

Loading status checks…

321c745

…nt_line is correctly updated

Added in missing parameter

Loading status checks…

12259aa

jdm requested changes Dec 9, 2016

View changes

karenher added 2 commits Dec 21, 2016

Made test check tuples of tokens and line numbers and changed other s…

Loading status checks…

d8322a5

…tyle issues

Added set_current_line function and test to treebuilder

Loading status checks…

8766e0a

jdm mentioned this pull request Dec 21, 2016

Added current_line variable to track current line servo/servo#14661

Closed

1 of 5 tasks complete

jdm requested changes Dec 22, 2016

View changes

Fixed style issues and test. Added '\n' to pop_except_from function c…

Loading status checks…

df8982b

…all in step function to account to make sure current_line is updated.

jdm mentioned this pull request Dec 28, 2016

Track current line when parsing #243

Merged

jdm closed this Dec 28, 2016

servo / html5ever

Added current_line variable to Tokenizer and test to check that curre… #240

Added current_line variable to Tokenizer and test to check that curre… #240

karenher commented Dec 1, 2016 •

edited by larsbergstrom

jdm left a comment

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.

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.

jdm left a comment

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.

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.

jdm commented Dec 28, 2016

servo / html5ever

Join GitHub today

Added current_line variable to Tokenizer and test to check that curre… #240

Added current_line variable to Tokenizer and test to check that curre… #240

Conversation

karenher commented Dec 1, 2016 • edited by larsbergstrom

jdm left a comment

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.

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.

jdm left a comment

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.

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.

jdm commented Dec 28, 2016

karenher commented Dec 1, 2016 •

edited by larsbergstrom