See bug 1273303. Emilio is going to take this.

Let's assume that's not horribly broken, I'll start posting patches for review, since I need to run now.

So, there's only a single failure from this patches, and I doubt it's because of these changes tbh, it's a single pixel difference in a jpeg image test (o.o). Seems reliable, though...

Here's a try run with the last commit: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb9394bfd4f421a1f835a5f4c9e8e565ecec9bf3

Comment on attachment 8871800 [details] Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades. https://reviewboard.mozilla.org/r/143258/#review147404 ::: layout/style/StyleSheet.cpp:441 (Diff revision 1) > // Ensure we're using the new rules. > - ClearRuleCascades(); > + if (CSSStyleSheet* geckoSheet = GetAsGecko()) { > + geckoSheet->ClearRuleCascades(); > + } Can you explain why we don't need to do this for Servo sheets? Is is that we've already done the work (marking the stylist dirty etc.)?

Comment on attachment 8871801 [details] Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets. https://reviewboard.mozilla.org/r/143260/#review147406 ::: commit-message-709de:6 (Diff revision 1) > +Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets. r?heycam > + > +We'll use it to invalidate stuff. > + > +MozReview-Commit-ID: Il3wO5JQh1Y > +Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io> Drop this (here and in the other patches). ::: layout/style/ServoStyleSet.cpp:134 (Diff revision 1) > mPresContext->RestyleManager()->PostRestyleEventForCSSRuleChanges( > - root, eRestyle_Subtree, nsChangeHint(0)); > + root, nsRestyleHint(0), nsChangeHint(0)); This won't do anything, will it? ServoRestyleManager::PostRestyleEvent returns early if the restyle hint and change hints are both zero. ::: layout/style/ServoStyleSet.cpp (Diff revision 1) > mAuthorStyleDisabled = aStyleDisabled; > + ForceAllStyleDirty(); > > - // If we've just disabled, we have to note the stylesheets have changed and > - // call flush directly, since the PresShell won't. > - if (mAuthorStyleDisabled) { Why don't we need to do this check any more?

Comment on attachment 8871802 [details] Bug 1357583: style: Add an initial version of the stylesheet invalidation code. https://reviewboard.mozilla.org/r/143262/#review147408 ::: servo/components/style/invalidation/mod.rs:5 (Diff revision 1) > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +//! A collection of invalidations due stylesheets added to the document. "due to" ::: servo/components/style/invalidation/mod.rs:19 (Diff revision 1) > +use selectors::parser::{Component, Selector}; > +use shared_lock::SharedRwLockReadGuard; > +use stylesheets::{CssRule, CssRules, Stylesheet}; > +use stylist::Stylist; > + > +/// A style scope represents a kind of subtree that may need to be restyled. s/A style scope/An invalidation scope/? Sounds a bit like <style scoped> or XBL style sheet scopes otherwise. ::: servo/components/style/invalidation/mod.rs:158 (Diff revision 1) > + /// We prefer id scopes to class scopes, and outermost scopes to innermost > + /// scopes (to reduce the amount of traversal we need to do). I guess I don't have a good sense of whether to do that, or to look at the rightmost ID/class and do more traversal. But seems fine to do this. And prioritising IDs sounds like a good idea. ::: servo/components/style/invalidation/mod.rs:187 (Diff revision 1) > + match scope { > + Some(s) => { > + self.invalid_scopes.insert(s); > + } > + None => { > + // If we didn't found a scope, any selector could match this, so find ::: servo/components/style/invalidation/mod.rs:211 (Diff revision 1) > + if !doc_rule.condition.evaluate(stylist.device()) { > + return false; // Won't apply anything else after this. > + } Why is this different from @media rules, where we return true? ::: servo/components/style/invalidation/mod.rs:267 (Diff revision 1) > + // guess. > + self.fully_invalid = true; > + } > + } > + > + return !self.fully_invalid Nit: drop "return".

Comment on attachment 8871803 [details] Bug 1357583: style: Hook up the invalidator in the StyleSheetSet. https://reviewboard.mozilla.org/r/143264/#review147412 ::: servo/components/style/gecko_string_cache/mod.rs:37 (Diff revision 1) > -macro_rules! local_name { > - ($s: tt) => { atom!($s) } > -} > +// macro_rules! local_name { > +// ($s: tt) => { atom!($s) } > +// } ? ::: servo/components/style/invalidation/mod.rs:109 (Diff revision 1) > /// Process style invalidations in a given subtree, that is, look for all > /// the relevant scopes in the subtree, and mark as dirty only the relevant > /// ones. > /// > /// Returns whether it invalidated at least one element's style. Something to think about for a followup, but I wonder if instead of expanding out the invalidations by traversing the subtree here before doing the main restyle traversal, we could instead have the invalidation set accessible from the restyle worker threads (in the shared style context), and check it during the normal traversal. It would mean that the dirty descendants bits don't quite accurately describe where we can stop traversing, but it would be nice to avoid this additional (serialized) traversal here. ::: servo/ports/geckolib/glue.rs:794 (Diff revision 1) > } > > #[no_mangle] > pub extern "C" fn Servo_StyleSet_FlushStyleSheets( > raw_data: RawServoStyleSetBorrowed, > - doc_element: RawServoElementBorrowedOrNull) > + doc_element: RawGeckoElementBorrowedOrNull) Maybe fix this in the earlier patch.

Comment on attachment 8871804 [details] Bug 1357583: Add an API to get the restyle generation from the pres context. https://reviewboard.mozilla.org/r/143266/#review147418

Comment on attachment 8871805 [details] Bug 1357583: Tidy the PostRestyleEventForCSSRuleChanges API. https://reviewboard.mozilla.org/r/143268/#review147420

Comment on attachment 8871806 [details] Bug 1357583: Add a bunch of logging, shortcuts, and look also at the rightmost selector while invalidating sheets. https://reviewboard.mozilla.org/r/143270/#review147422

Comment on attachment 8871807 [details] Bug 1357583: Test. https://reviewboard.mozilla.org/r/143272/#review147424 Looks good. Maybe add some perf-reftests for this too. ::: layout/style/test/test_stylesheet_additions.html:25 (Diff revision 1) > +<style id="target" disabled></style> > +<script> > +SimpleTest.waitForExplicitFinish(); > +const utils = SpecialPowers.getDOMWindowUtils(window); > +const TESTS = [ > + { selector: ".unexistentClassScope", restyle: false }, s/unexisting/nonexistent/g

Comment on attachment 8871909 [details] Bug 1357583: style: Recurse into @import rules when looking at the added rules. https://reviewboard.mozilla.org/r/143430/#review147426

Comment on attachment 8871960 [details] Bug 1357583: style: Make effective_rules return an iterator, stop refcounting the Device. https://reviewboard.mozilla.org/r/143484/#review147432 ::: servo/components/style/stylist.rs:435 (Diff revision 2) > - // Cheap `Arc` clone so that the closure below can borrow `&mut Stylist`. > + for rule in stylesheet.effective_rules(&self.device, guard) { > - let device = self.device.clone(); Did you inline the style rule processing function calls to avoid this? The out of line functions probably make this function a bit more manageable...

Comment on attachment 8871977 [details] Bug 1357583: Avoid unconditionally popping under RulesIterator. https://reviewboard.mozilla.org/r/143510/#review147436 ::: servo/components/style/stylesheets.rs:991 (Diff revision 1) > + let mut nested_iter = self.stack.last_mut().unwrap(); > + rule = match nested_iter.next() { > - Some(r) => r, > + Some(r) => r, > - None => continue, > + None => { > + pop = true; > + continue > + } > - }; > + }; Maybe rename "pop" to "nested_iter_finished"? I guess because of lifetime stuff you can't just do the popping in here, as soon as you detect the nested iterator is finished.

Comment on attachment 8871800 [details] Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades. https://reviewboard.mozilla.org/r/143258/#review147404 > Can you explain why we don't need to do this for Servo sheets? Is is that we've already done the work (marking the stylist dirty etc.)? So, I removed this because it wasn't clear at all it's needed. Basically every change that makes us change the set of selectors/rules we match is covered by the presshell notifications. I understand this may be needed in Gecko because the selectors are not refcounted (IIUC), but this is not the case in Servo.

Comment on attachment 8871801 [details] Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets. https://reviewboard.mozilla.org/r/143260/#review147406 > Drop this (here and in the other patches). Yikes, I need to make `git format-patch` smarter than auto-signing off. > This won't do anything, will it? ServoRestyleManager::PostRestyleEvent returns early if the restyle hint and change hints are both zero. Right, the idea was that it did set the mRestyleForCSSRuleChanges boolean. But this doesn't force a style flush to happen, nor this patch implements the "dirtyness" bit when flushing, so as written it's basically wrong. It's fixed in the following patches though. I can squash this commit somehow into another one so that it's not broken. > Why don't we need to do this check any more? We didn't need it in the first place. I think this was intended as an "optimization"? I never understood that comment about JS and the presshell notifications (neither did bz btw). It doesn't optimize anything basically, and only relied on other part of the code (which one, it isn't clear to me) to mark the stylist as dirty. I removed it to be explicit.

Comment on attachment 8871800 [details] Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades. https://reviewboard.mozilla.org/r/143258/#review147404 > So, I removed this because it wasn't clear at all it's needed. Basically every change that makes us change the set of selectors/rules we match is covered by the presshell notifications. > > I understand this may be needed in Gecko because the selectors are not refcounted (IIUC), but this is not the case in Servo. OK, can you please add a comment here explaining this.

Comment on attachment 8871800 [details] Bug 1357583: Remove ServoStyleSheet::ClearRuleCascades. https://reviewboard.mozilla.org/r/143258/#review147574

Comment on attachment 8871801 [details] Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets. https://reviewboard.mozilla.org/r/143260/#review147406 > Right, the idea was that it did set the mRestyleForCSSRuleChanges boolean. But this doesn't force a style flush to happen, nor this patch implements the "dirtyness" bit when flushing, so as written it's basically wrong. It's fixed in the following patches though. I can squash this commit somehow into another one so that it's not broken. Ah, yes, I did see in the later patches this was changed. Not a big deal, if you want to leave this as is. > We didn't need it in the first place. > > I think this was intended as an "optimization"? I never understood that comment about JS and the presshell notifications (neither did bz btw). > > It doesn't optimize anything basically, and only relied on other part of the code (which one, it isn't clear to me) to mark the stylist as dirty. > > I removed it to be explicit. OK.

Comment on attachment 8871801 [details] Bug 1357583: Ensure we send the document element to Servo when flushing stylesheets. https://reviewboard.mozilla.org/r/143260/#review147578

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3deb86de58ce Remove ServoStyleSheet::ClearRuleCascades. r=heycam https://hg.mozilla.org/integration/autoland/rev/d279a19a1e15 Ensure we send the document element to Servo when flushing stylesheets. r=heycam https://hg.mozilla.org/integration/autoland/rev/386f7a3132e7 style: Hook up the invalidator in the StyleSheetSet. r=heycam https://hg.mozilla.org/integration/autoland/rev/142539198f30 Tidy the PostRestyleEventForCSSRuleChanges API. r=heycam https://hg.mozilla.org/integration/autoland/rev/8f0667eca06e Test. r=heycam

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/07778bb17a4a Add an API to get the restyle generation from the pres context. r=heycam

https://hg.mozilla.org/mozilla-central/rev/3deb86de58ce https://hg.mozilla.org/mozilla-central/rev/d279a19a1e15 https://hg.mozilla.org/mozilla-central/rev/386f7a3132e7 https://hg.mozilla.org/mozilla-central/rev/142539198f30 https://hg.mozilla.org/mozilla-central/rev/8f0667eca06e https://hg.mozilla.org/mozilla-central/rev/07778bb17a4a