Bug 205264 – Resolve dynamic media queries without reconstructing RuleSets

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Antti Koivisto]

Switch media query affected rules on and off dynamically.

[Antti Koivisto]

Created attachment 385758 [details]
patch

[Emilio Cobos Álvarez (:emilio)]

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.

[Antti Koivisto]

> @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.

[Antti Koivisto]

> 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 :)

[Emilio Cobos Álvarez (:emilio)]

(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!

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 385758 [details]
patch

r=me

[Antti Koivisto]

> 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.

[Antti Koivisto]

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.

[Sam Weinig]

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"

[Antti Koivisto]

Created attachment 385864 [details]
patch

[WebKit Commit Bot]

Comment on attachment 385864 [details]
patch

Clearing flags on attachment: 385864

Committed r253616: <https://trac.webkit.org/changeset/253616>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58000763>