Switch media query affected rules on and off dynamically.
Created attachment 385758 [details] patch
Comment on attachment 385758 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=385758&action=review This looks fancy to me (I took the CC as an invitation to take a look, but please let me know if I took it wrong). I can't r+ it myself as I'm not 100% sure of the above and I'm not technically a reviewer, but this looks good otherwise. The only thing I haven't double-checked and would take me more to check is whether this causes WebKit to overinvalidate for class/id/attribute changes for stuff that doesn't apply. So like whether: @media (width: 0) { .myclass * { ... } } Causes "myclass" changes to restyle the whole subtree even if the feature doesn't match. Seems it probably would, but it might be ok. > Source/WebCore/css/MediaQueryEvaluator.cpp:149 > { Probably assert that if mode is AlwaysMatchDynamic, then dynamicResults is not null? > Source/WebCore/style/RuleSet.cpp:308 > + mediaQueryCollector.didMutateResolver(); There's nothing preventing you in the future to take the same approach for keyframes and font-face and such, right? (as in, track them in the dynamic results, and enable / disable them on demand). But this seems fine for now, and it is certainly hard to handle. > Source/WebCore/style/RuleSet.cpp:335 > + auto mediaQueryCollector = MediaQueryCollector { evaluator, true }; nit: May be worth to make collectDynamic an enum class, so this reads like CollectDynamic::Yes or such. But if it makes the surrounding code too ugly then probably never mind. > Source/WebCore/style/RuleSet.h:80 > + Vector<size_t> affectedRulePositions { }; nit: I think you can just use Vector<size_t> affectedRulePositions; and same with the other vectors here.
> @media (width: 0) { > .myclass * { ... } > } > > Causes "myclass" changes to restyle the whole subtree even if the feature > doesn't match. Seems it probably would, but it might be ok. With this patch we still just re-resolve everything. I have a followup that will construct invalidation rulesets for each query. We'll only invalidate the associated elements and only when the query result changes. > There's nothing preventing you in the future to take the same approach for > keyframes and font-face and such, right? (as in, track them in the dynamic > results, and enable / disable them on demand). Absolutely. This is just a shortcut to get the common case covered (none of the sites I tried hit this case). > > Source/WebCore/style/RuleSet.h:80 > > + Vector<size_t> affectedRulePositions { }; > > nit: I think you can just use Vector<size_t> affectedRulePositions; and same > with the other vectors here. Explicit { } here allows compact struct initialisation auto mediaQueryCollector = MediaQueryCollector { evaluator, canUseDynamicMediaQueryResolution }; without providing explicit constructor.
> This looks fancy to me (I took the CC as an invitation to take a look, but > please let me know if I took it wrong). And yes, I CC'd to get comments :)
(In reply to Antti Koivisto from comment #3) > > @media (width: 0) { > > .myclass * { ... } > > } > > > > Causes "myclass" changes to restyle the whole subtree even if the feature > > doesn't match. Seems it probably would, but it might be ok. > > With this patch we still just re-resolve everything. I have a followup that > will construct invalidation rulesets for each query. We'll only invalidate > the associated elements and only when the query result changes. Sure, when media queries change we just re-resolve everything, and the rulesets for each query would fix that indeed. My question was more about whether this patch changes the RuleFeatures that end up being used for class / id / etc invalidations. Which it would, but your followup would address that as well. So that sounds great. > Explicit { } here allows compact struct initialisation > > auto mediaQueryCollector = MediaQueryCollector { evaluator, > canUseDynamicMediaQueryResolution }; > > without providing explicit constructor. Fancy, TIL, thanks!
Comment on attachment 385758 [details] patch r=me
> My question was more about whether this patch changes the RuleFeatures that > end up being used for class / id / etc invalidations. Which it would, but > your followup would address that as well. So that sounds great. Right, yes. These rules end up in class etc invalidation rulesets. I don't think that is a major concern in practice (requires costly and unique selectors inside non-matching queries) but you can certainly construct a case where it would be significant. I'll address it later if needed.
Comment on attachment 385758 [details] patch Oh sorry, I thought you were reviewer already when I told to set the r+. Lets find someone else for formal review.
Comment on attachment 385758 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=385758&action=review > Source/WebCore/ChangeLog:10 > + to re-resolve full document style. This may happen frequently, for example when resizing window on a reponsive "reponsive" -> "responsive" > Source/WebCore/ChangeLog:11 > + we site. "we site" -> "web site" > Source/WebCore/ChangeLog:15 > + (like view resize). This allows us to avoid throwing away anything during common screnarios. "screnarios" -> "scenarios"
Created attachment 385864 [details] patch
Comment on attachment 385864 [details] patch Clearing flags on attachment: 385864 Committed r253616: <https://trac.webkit.org/changeset/253616>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58000763>