Xhtml5ever

[Ygg01]

Ok, this is a large one.

Fixes #266, fixes #261, fixes #210.

It moves html5ever into separate folder, renames html5ever macros markup5ever and stores common code there.

Here is short summary of what I know is and isn't done.

• Make every crate in the repo use a single workspace
• Make sure Travis-CI is running every test
• Rename the the html5ever_atoms crate to markup5ever and update html5ever and xml5ever to use it.
• Increment version numbers
• Make it so that users of either html5ever or xml5ever don’t need to have an explicit dependency to markup5ever
• Export QualName QualName should be re-exported #210
• let markup5ever generate entities.json html and xml named entity tables should be shared #261
• Move TokenSink to markup5ever
• Move TreeSink to markup5ever
• Move BufferQueue to markup5ever
• Move SmallCharSet to markup5ever
• Deal with driver.rs

---

This change is 

[Ygg01]

Possible issues:

• better organization of pub use.
• elem_name and elem_name_ref (and same node/same_node_ref) might be redundant. But I didn't want to change elem_name to use references, because possible unseen issues
• the code could use CargoFmt and Clippy

[SimonSapin]

Wow, thanks a lot! I’ll look at the diffs later but based just on the commit messages this looks great.

A couple notes:

• Move TokenSink to markup5ever

As I’m mentioned I don’t think this is important. When an application wants to support both XML and HTML it will most likely use each parser as a whole, not the tokenizer or the tree builder individually. So while it is useful to make TreeSink a shared/compatible interface (because that’s the output of the parser as a whole), TokenSink on the other hand is an internal implementation detail that is irrelevant to such an application.

• Deal with driver.rs

There is stuff to do here, but I think it’s mostly independent of what’s in this PR at this point. So don’t worry about it :)

-// Copyright 2014 The html5ever Project Developers. See the
+// Copyright 2016 The html5ever Project Developers. See the

I think this should be a range: Copyright 2014-2017.

[Ygg01]

My grep-fu failed me on first attempt. I had to rebase some Copyright fixes. I apologize for the inconvenience.

Thank you for kind words. I hope reviewing it won't be too difficult. BTW I won't be near computer for a few days, so I might take longer to respond/fix.

[SimonSapin]

BTW I won't be near computer for a few days, so I might take longer to respond/fix.

That’s fine. We’re not in a hurry with this, and this repository doesn’t see much activity at the moment so this will hopefully not bitrot.

However, if you prefer, I can take over the PR and fix up some minor things and land it in your absence. In that case I have one question (so far):

In scripts/travis-build.sh:

# This is a hack until cargo doc lands on stable
cargo doc -p html5ever
cargo doc -p xml5ever

What do you mean, “until cargo doc lands on stable”?

[Ygg01]

Yeah, I forgot to fix that comment. I meant a hack until cargo doc --all works on stable. Right now its nightly, I believe?

EDIT: Hmm. It seems my rustc was old, the command should work on stable now.

[SimonSapin]

Re cargo doc --all, ok.

---

Reviewed 94 of 98 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 9 of 9 files at r6, 13 of 13 files at r7, 14 of 14 files at r8, 11 of 12 files at r9, 8 of 8 files at r10, 5 of 5 files at r11, 26 of 26 files at r12, 10 of 10 files at r13, 7 of 7 files at r14, 56 of 56 files at r15.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

markup5ever/Cargo.toml, line 27 at r8 (raw file):

phf_codegen = "0.7.3"
rustc-serialize = "0.3.15"
syn = { version = "0.11", features = ["full", "visit"] }

It looks like syn isn’t used in m5e?

---

markup5ever/lib.rs, line 20 at r12 (raw file):

macro_rules! qualname {
    ("", $local:tt) => {
        ::markup5ever::QualName {

Why not $crate here?

---

scripts/travis-build.sh, line 25 at r10 (raw file):

fi

# This is a hack until cargo doc lands on stable

What do you mean, “until cargo doc lands on stable”?

---

xml5ever/Cargo.toml, line 21 at r4 (raw file):

rust-lang/cargo#1512

This issue is "plugins not working in doc tests". Since we’re not using compile plugins anymore this is no longer relevant. And the issue is fixed anyway.

If there is any doctest in either library we should enable them back. If not, we should keep doctest = false so that cargo test doesn’t spend time to collect zero doctests, but change the comment to say that instead.

---

xml5ever/src/rcdom.rs, line 43 at r12 (raw file):

    /// A script element and its "already started" flag.
    /// https://html.spec.whatwg.org/multipage/#already-started
    Script(bool),

While we’re touching this, let’s used a struct-like variant to name the field: Script { already_started: bool }

---

xml5ever/src/rcdom.rs, line 110 at r12 (raw file):

#[allow(trivial_casts)]
fn same_node(x: &Handle, y: &Handle) -> bool {

Let’s remove this and use Rc::ptr_eq instead. Unless we want to support older Rust versions? It’s added in 1.17.

---

xml5ever/tests/tokenizer.rs, line 1 at r5 (raw file):

#![cfg_attr(feature = "unstable", feature(start, test))]

I think this can be removed as well.

---

Comments from Reviewable

[Ygg01]

scripts/travis-build.sh, line 25 at r10 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

What do you mean, “until cargo doc lands on stable”?

It's an error on my part, caused by old rustc. cargo test --all works on stable, but my older version didn't allow.

---

Comments from Reviewable

[Ygg01]

markup5ever/Cargo.toml, line 27 at r8 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

It looks like syn isn’t used in m5e?

Hm, I was under the impression, that it was used, since no warning was issued. But you are right.

---

Comments from Reviewable

[Ygg01]

xml5ever/Cargo.toml, line 21 at r4 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

rust-lang/cargo#1512

This issue is "plugins not working in doc tests". Since we’re not using compile plugins anymore this is no longer relevant. And the issue is fixed anyway.

If there is any doctest in either library we should enable them back. If not, we should keep doctest = false so that cargo test doesn’t spend time to collect zero doctests, but change the comment to say that instead.

Enable this in html5ever, as well I assume?

---

Comments from Reviewable

[SimonSapin]

Reviewed 2 of 98 files at r1, 3 of 9 files at r6, 1 of 8 files at r10, 1 of 10 files at r13, 1 of 1 files at r16, 6 of 6 files at r17, 2 of 2 files at r18, 2 of 2 files at r19.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

markup5ever/interface/mod.rs, line 139 at r17 (raw file):

    #[test]
    fn qualname_macro() {
        assert_eq!(qualname!("", ""), QualName { ns: ns!(), local: local_name!(""), prefix: None });

qualname! is sometimes used as a pattern. I don’t know that we should be checking that the prefix is None in those case… We may want separate macros: one with a prefix: _ pattern, one for expressions. I’ll survey usage of the macro, in both h/x5e and Servo.

---

Comments from Reviewable

[Ygg01]

@SimonSapin. What to do about qualname?

[SimonSapin]

This PR looks good, thanks! I added more stuff on top at: Ygg01#1. That includes taking care of qualname! (by removing most of its usage and making it private).

[SimonSapin]

@bors-servo r=SimonSapin,Ygg01

[bors-servo]

📌 Commit 50bcef5 has been approved by SimonSapin,Ygg01

[bors-servo]

⌛ Testing commit 50bcef5 with merge cee43bd...

[bors-servo]

☀️ Test successful - status-travis
Approved by: SimonSapin,Ygg01
Pushing cee43bd to master...

[Ygg01]

Review status: 49 of 90 files reviewed at latest revision, 2 unresolved discussions.

---

xml5ever/src/rcdom.rs, line 110 at r12 (raw file):

Previously, SimonSapin (Simon Sapin) wrote…

Let’s remove this and use Rc::ptr_eq instead. Unless we want to support older Rust versions? It’s added in 1.17.

AFAIK no need for support.

---

Comments from Reviewable