Dirty elements whose selectors are affected by sibling changes

[mbrubeck]

This fixes incremental layout of nodes that match pseudo-class selectors such as :first-child, :nth-child, :last-child, :first-of-type, etc. Fixes #8191 and other intermittent layout bugs.

This code is based on the following flags from Gecko:
https://hg.mozilla.org/mozilla-central/file/e1cf617a1f28/dom/base/nsINode.h#l134

Depends on servo/rust-selectors#71. r? @SimonSapin

There are a couple of TODO items in this commit, but I'd appreciate feedback on the general approach before I finish it up. (Also, if someone who knows more than I do could give some advice about atomic orderings...)

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!
• These commits modify layout and script code, but no tests are modified. Please consider adding a test!

[KiChjang]

If this fixes #8191, then you may want to re-enable the test again, since I have disabled it in #9716.

[mbrubeck]

Re-enabled two disabled tests, and finished one of the TODO items.

[nox]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

components/script/dom/node.rs, line 2403 [r1] (raw file):
Why "previously-existing"?

---

components/script/dom/node.rs, line 2438 [r1] (raw file):
You didn't cover ChildrenMutation::Insert: just because this is not an Append doesn't mean a new first Element didn't get inserted.

Also I would replace the catch-all _ => None by:

ChildrenMutation::Insert(…) => …,
ChildrenMutation::Replace { prev: None, next: None, .. } => unreachable!(),
ChildrenMutation::ReplaceAll { .. } => None,

---

Comments from the review on Reviewable.io

[mbrubeck]

Addressed review comments. Note: This still depends on servo/rust-selectors#72 and will need a cargo update after that is published.

---

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

---

components/script/dom/node.rs, line 2403 [r1] (raw file):
That was redundant; removed it.

---

components/script/dom/node.rs, line 2438 [r1] (raw file):
Good catch, thanks! The same applies to Replace { prev: Some, next: Some }. Both are handled correctly, now.

---

Comments from the review on Reviewable.io

[mbrubeck]

Rebased, cargo updated, and re-enabled two more tests. This is now ready to merge after r+.

[pcwalton]

This looks good to me. Exciting!

[KiChjang]

Fails tidy:

./components/script/dom/element.rs:69: use statement is not in alphabetical order

    expected: selectors::matching::{HAS_SLOW_SELECTOR, HAS_EDGE_CHILD_SELECTOR, HAS_SLOW_SELECTOR_LATER_SIBLINGS}

    found: selectors::matching::{DeclarationBlock, ElementFlags, matches}

[KiChjang]

Also fails a couple of interesting unit tests:

failures:

---- size_of::size_div stdout ----

    thread 'size_of::size_div' panicked at 'Your changes have increased the stack size of commonly used DOM struct HTMLDivElement from 320 to 328. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in script/tests.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:44

note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- size_of::size_element stdout ----

    thread 'size_of::size_element' panicked at 'Your changes have increased the stack size of commonly used DOM struct Element from 304 to 312. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in script/tests.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:42

---- size_of::size_htmlelement stdout ----

    thread 'size_of::size_htmlelement' panicked at 'Your changes have increased the stack size of commonly used DOM struct HTMLElement from 320 to 328. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in script/tests.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:43

---- size_of::size_span stdout ----

    thread 'size_of::size_span' panicked at 'Your changes have increased the stack size of commonly used DOM struct HTMLSpanElement from 320 to 328. These structs are present in large quantities in the DOM, and increasing the size may dramatically affect our memory footprint. Please consider choosing a design which avoids this increase. If you feel that the increase is necessary, update to the new size in script/tests.rs.', /home/travis/build/servo/servo/tests/unit/script/size_of.rs:45

[mbrubeck]

Fixed the tidy error, and updated the expected sizes in size_of.rs. I think the size increase is basically unavoidable; we are adding new info to Element that we didn't have before. We could cram the info into NodeFlags instead but it only has space for one more flag, and is not thread-safe.

[nox]

@bors-servo r+

---

Reviewed 1 of 1 files at r2, 2 of 2 files at r3, 7 of 7 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 9739189 has been approved by nox

[bors-servo]

⌛ Testing commit 9739189 with merge 438b3a4...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt