I thought we had a bug on file on this, but we don't, so let's do it. Right now we post a full restyle every time anything that may change media query evaluation results changes. We should definitely get that fixed before shipping, since I'm pretty sure it'll make window resizes unacceptably slow.

I finally got time to write this. Patches incoming. I doubt patches pass tests without the mUsesVieportUnits lie, but I'll do that as a Part 0, I think. I need to run now though.

Comment on attachment 8873956 [details] Bug 1357461: style: Move the stylesheet invalidation code to another submodule. https://reviewboard.mozilla.org/r/145372/#review149456

Comment on attachment 8873957 [details] Bug 1357461: Make RulesIterator not iterate over rules for which we don't process nested rules. https://reviewboard.mozilla.org/r/145374/#review149458 Maybe document on NestedRuleIterationCondition that these functions mean to skip the rule as well as their nested rules?

Comment on attachment 8873958 [details] Bug 1357461: Cache effective media query results. https://reviewboard.mozilla.org/r/145376/#review149460 ::: servo/components/style/invalidation/media_queries.rs:16 (Diff revision 1) > +/// NOTE: It happens to be the case that all the media lists we care about > +/// happen to have a stable address, so we can just use an opaque pointer to > +/// represent them. Is it possible that we can destroy a MediaRule (for example) and create a new one soon after, and we think we have the same MediaListKey? Are we guaranteed that we'll invalidate any of the MediaListKeys we've stored by clearing the Stylist's EffectiveMediaQueryResults when rules/sheets are removed? ::: servo/components/style/stylist.rs:783 (Diff revision 1) > - let mq = stylesheet.media.read_with(guard); > - if mq.evaluate(&self.device, self.quirks_mode) != mq.evaluate(&device, self.quirks_mode) { > + let features_changed = self.media_features_changed( > + stylesheets.iter(), > + guard > + ); Nit: not really a fan of wrapping arguments like this when we can easily fit within our 99 (or 119 at a stretch) column limit on one line. I think it makes it harder to read. ::: servo/components/style/stylist.rs:820 (Diff revision 1) > - guard); > + guard > + ); Nit: neither this. It's not some new rustfmt rule is it?

Comment on attachment 8873959 [details] Bug 1357461: Add some logging. https://reviewboard.mozilla.org/r/145378/#review149462

Comment on attachment 8873960 [details] Bug 1357461: Ensure the device is up-to-date before evaluating media queries. https://reviewboard.mozilla.org/r/145380/#review149464 ::: servo/ports/geckolib/glue.rs:762 (Diff revision 1) > + // We need to ensure the default computed values are up to date though, > + // because those can influence the result of media query evaluation. > + let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); > + data.stylist.device_mut().reset(); Is there a way we can avoid recomputing the default computed values, but ensuring they're up to date? I think we'll call into this function for every window resize event, so it would be good to avoid doing work that we don't need to. ::: servo/ports/geckolib/glue.rs:1435 (Diff revision 1) > - data.reset_device(&guard); > + data.stylesheets.force_dirty(); > + data.flush_stylesheets::<GeckoElement>(&guard, None); What's the effect of this change?

Comment on attachment 8873961 [details] Bug 1357461: Ensure not to increment the restyle generation if we haven't restyled after all. https://reviewboard.mozilla.org/r/145382/#review149466 ::: layout/base/ServoRestyleManager.h:118 (Diff revision 2) > /** > * Performs post-Servo-traversal processing on this element and its descendants. > */ > - void ProcessPostTraversal(Element* aElement, > + bool ProcessPostTraversal(Element* aElement, > nsStyleContext* aParentContext, > ServoStyleSet* aStyleSet, > nsStyleChangeList& aChangeList); > > struct TextPostTraversalState; > - void ProcessPostTraversalForText(nsIContent* aTextNode, > + bool ProcessPostTraversalForText(nsIContent* aTextNode, > nsStyleChangeList& aChangeList, > TextPostTraversalState& aState); Please comment here what the return value means.

Comment on attachment 8873960 [details] Bug 1357461: Ensure the device is up-to-date before evaluating media queries. https://reviewboard.mozilla.org/r/145380/#review149464 > Is there a way we can avoid recomputing the default computed values, but ensuring they're up to date? I think we'll call into this function for every window resize event, so it would be good to avoid doing work that we don't need to. I'll file a bug for this. > What's the effect of this change? Which change? This is effectively doing the same we were doing before, though now we don't need to call device.reset() it didn't make a lot of sense to have a `reset_device` function, so I instead removed it and inlined it here, since there was only one caller. If there can be other ways that the default computed values may change, we may need to do it here too.

Comment on attachment 8873960 [details] Bug 1357461: Ensure the device is up-to-date before evaluating media queries. https://reviewboard.mozilla.org/r/145380/#review149464 > Which change? This is effectively doing the same we were doing before, though now we don't need to call device.reset() it didn't make a lot of sense to have a `reset_device` function, so I instead removed it and inlined it here, since there was only one caller. If there can be other ways that the default computed values may change, we may need to do it here too. OK, I just didn't know (or check) whether it was equivalent to the old code.

Comment on attachment 8873960 [details] Bug 1357461: Ensure the device is up-to-date before evaluating media queries. https://reviewboard.mozilla.org/r/145380/#review149474

Comment on attachment 8873958 [details] Bug 1357461: Cache effective media query results. https://reviewboard.mozilla.org/r/145376/#review149460 > Is it possible that we can destroy a MediaRule (for example) and create a new one soon after, and we think we have the same MediaListKey? Are we guaranteed that we'll invalidate any of the MediaListKeys we've stored by clearing the Stylist's EffectiveMediaQueryResults when rules/sheets are removed? Yes, I think so, since as of right now rule removal and stylesheet removal both trigger a full rebuild. We could do better, and we'd need to ensure we remove the keys in that case. > Nit: neither this. It's not some new rustfmt rule is it? Yes, see https://github.com/rust-lang-nursery/fmt-rfcs and the related issues. rustfmt is switching to block-formatting for arguments.

Comment on attachment 8874094 [details] Bug 1357461: Assume viewport units are used on resize. https://reviewboard.mozilla.org/r/145554/#review149480 ::: layout/base/nsPresContext.cpp:2077 (Diff revision 1) > - if (mUsesViewportUnits && mPendingViewportChange) { > + if (mPendingViewportChange && ( > + (mUsesViewportUnits || mDocument->IsStyledByServo()))) { Nit: drop the extra set of parens (the one surrounding the second subexpression starting on the first line).

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29c25155b8485716fa0b0ddde6abd7afa72d8e1 PR: https://github.com/servo/servo/pull/17147

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/54e9145b8892 Cache effective media query results. r=heycam https://hg.mozilla.org/integration/autoland/rev/e0477439b222 Ensure not to increment the restyle generation if we haven't restyled after all. r=heycam https://hg.mozilla.org/integration/autoland/rev/6ddc4c39388e Assume viewport units are used on resize. r=heycam

https://hg.mozilla.org/mozilla-central/rev/54e9145b8892 https://hg.mozilla.org/mozilla-central/rev/e0477439b222 https://hg.mozilla.org/mozilla-central/rev/6ddc4c39388e