Systematically ignore tabs and newlines, per spec change

[SimonSapin]

whatwg/url@7b40216
whatwg/url#101

---

This change is

[asajeffrey]

This seems like a lot of extra complexity to handle an uncommon case (URLs with tabs or spaces). It seems like there should be a simpler way to do this, something like making Input implement a peekable and perhaps cloneable iterator interface.

Am I right that one of the goals is to keep this as a one-pass algorithm, which is why the obvious brute force scan of the input for tabs and spacing before parsing (and filtering/collecting it if any are found) isn't acceptable?

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

src/lib.rs, line 594 [r1] (raw file):
It's a pity this (and the other functions like it) isn't just input.chars().filter(is_url_char), possibly with some stuff about stripping off leading/trailing characters.

---

src/parser.rs, line 130 [r1] (raw file):
I'm a bit surprised we need this many helper methods. Does the URL parser need that many methods that aren't in some mix of Peekable<Iterator<char>>? and IntoIterator?

---

src/parser.rs, line 326 [r1] (raw file):
Why this change?

---

src/parser.rs, line 340 [r1] (raw file):
This is doing a lot more allocation than the original.

---

src/parser.rs, line 351 [r1] (raw file):
The code seems more complex now, is there a way to keep the simplicity of the original?

---

src/parser.rs, line 621 [r1] (raw file):
More allocation.

---

src/parser.rs, line 678 [r1] (raw file):
The new code is more complex.

---

src/parser.rs, line 745 [r1] (raw file):
And more complexity.

---

Comments from Reviewable

[SimonSapin]

The complexity was already there, just (inconsistently) spread in various parts of the parser that used to each have code to ignore these characters. But with this spec change I think it would be unreasonable complex to ignore them manually everywhere, for example between the slashes of http://.

something like making Input implement a peekable and perhaps cloneable iterator interface.

That’s kinda what this PR is all about? (iterator.clone().next() is the same as peek when clone is cheap.)

Am I right that one of the goals is to keep this as a one-pass algorithm,

Yes. Or maybe filtering and collecting into a String is acceptable, but it seemed undesirable. I don’t know.

---

Review status: 5 of 6 files reviewed at latest revision, 8 unresolved discussions.

---

src/lib.rs, line 594 [r1] (raw file):
That’s… exactly what Input is?

---

src/parser.rs, line 130 [r1] (raw file):
Sure, you can always manually inline all these helpers instead of having them in methods. But that’s overall more code, mostly duplicated?

---

src/parser.rs, line 326 [r1] (raw file):
str::find returns a byte index that can be used for indexing. Input supports neither indexing nor .len().

---

src/parser.rs, line 340 [r1] (raw file):
Only for users that want the full details of "syntax violations", which so far no-one has ever used as far as I know.

---

src/parser.rs, line 351 [r1] (raw file):
If this comment is about this specific line, I don’t see how it is more complex. If it is about this PR in general… do you have a more concrete suggestion?

---

src/parser.rs, line 621 [r1] (raw file):
Still only when logging "syntax violations".

---

src/parser.rs, line 678 [r1] (raw file):
¯_(ツ)_/¯

This PR is not just an internal refactoring. It’s about implementing a change (linked from the PR message) in the specification’s requirements.

If you look at individual commits, the first one "Parse based on iterators" is a refactoring that doesn’t change the behavior, but it only exists to enable the behavior change in the second commit "Systematically ignore tabs and newlines".

---

src/parser.rs, line 745 [r1] (raw file):
See previous comment.

---

Comments from Reviewable

[asajeffrey]

Archiving an IRC conversation: http://logs.glob.uno/?c=mozilla%23servo&s=26+Apr+2016&e=26+Apr+2016#c415136

[asajeffrey]

I think at this point we're into style differences. You can r=me. Squash the commits?

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/parser.rs, line 107 [r2] (raw file):
Much nicer!

---

Comments from Reviewable

[SimonSapin]

Commits are as intended.

@bors-servo r+

[bors-servo]

📌 Commit cd7d83b has been approved by SimonSapin

[bors-servo]

⌛ Testing commit cd7d83b with merge 28d691c...

[bors-servo]

☀️ Test successful - travis