1466008 - pref'ing off properties doesn't pref off their will-change support

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://dbaron.org/css/test/2018/stacking-context-z-order shows that:
 * `contain` support is preffed off
 * but we still support `will-change: contain`

It seems like if we have support for a property pref'd off, we shouldn't be paying attention to the flags of what it does in 'will-change'.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Will take some time to clean it up while at it.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1466008: Make will-change honor prefs properly, and clean it up while at it.

Will add a test, though in JSConf right now...

Review commit: https://reviewboard.mozilla.org/r/248432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248432/

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8982463 [details]
Bug 1466008: Make will-change honor prefs properly, and clean it up while at it.

https://reviewboard.mozilla.org/r/248432/#review254636

::: servo/components/style/properties/gecko.mako.rs:3520
(Diff revision 1)
> -            }
> -
> -            bitfield as u8
> -        }
> -
>          self.gecko.mWillChangeBitField = 0;

You can probably move this line into the `T::Auto` branch now.

::: servo/components/style/properties/gecko.mako.rs:3530
(Diff revision 1)
> -                        Gecko_AppendWillChange(&mut self.gecko, feature.0.as_ptr());
> -                    }
> -
> -                    if let Ok(prop_id) = PropertyId::parse(&feature.0.to_string()) {
> +                        Gecko_AppendWillChange(
> +                            &mut self.gecko,
> +                            feature.0.as_ptr(),
> +                        );

It doesn't seem to me this should be broken into multiple lines... Actually, rustfmt would do the reverse, although file is not rustfmt-able.

::: servo/components/style/values/specified/box.rs:408
(Diff revision 1)
>  }
>  
> +bitflags! {
> +    /// The change bits that we care about.
> +    ///
> +    /// These need to be in sync with NS_STYLE_WILL_CHANGE_*.

Maybe we can use some static_assert technique like [1] to ensure this.

[1] https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/servo/components/style/properties/gecko.mako.rs#1536-1548

::: servo/components/style/values/specified/box.rs:447
(Diff revision 1)
> +    let id = match PropertyId::parse(ident) {
> +        Ok(id) => id,
> +        Err(..) => return WillChangeBits::empty(),
> +    };
> +
> +    if !id.allowed_in(context) {
> +        return WillChangeBits::empty();
> +    }

This makes me think we probably should have this check embeded into `PropertyId::parse`, so that every caller of it has to consider what set of properties they really want to accept. That should be better than patching each usage with an extra `allowed_in` after we find this kind of issues.

Mind filing a followup bug?

::: servo/components/style/values/specified/box.rs:458
(Diff revision 1)
> +            let mut flags = WillChangeBits::empty();
> +            for longhand in shorthand.longhands() {
> +                flags |= change_bits_for_longhand(longhand);
> +            }
> +            flags

May look better this way?
```rust
shorthand.longhands().fold(WillChangeBits::empty(), |flags, p| {
    flags | change_bits_for_longhand(p)
})
```

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8982463 [details]
Bug 1466008: Make will-change honor prefs properly, and clean it up while at it.

https://reviewboard.mozilla.org/r/248432/#review254636

> Maybe we can use some static_assert technique like [1] to ensure this.
> 
> [1] https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/servo/components/style/properties/gecko.mako.rs#1536-1548

Maybe, though I'd need to enumerate the flags yet in another place...

> This makes me think we probably should have this check embeded into `PropertyId::parse`, so that every caller of it has to consider what set of properties they really want to accept. That should be better than patching each usage with an extra `allowed_in` after we find this kind of issues.
> 
> Mind filing a followup bug?

So, I tried to do something like that before and didn't end up liking the result... The issue with that not all callers have a ParserContext available, and those should probably use enabled_for_all_content...

Probably we can change the semantics to that, will look into it.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8982463 [details]
Bug 1466008: Make will-change honor prefs properly, and clean it up while at it.

https://reviewboard.mozilla.org/r/248432/#review254636

> So, I tried to do something like that before and didn't end up liking the result... The issue with that not all callers have a ParserContext available, and those should probably use enabled_for_all_content...
> 
> Probably we can change the semantics to that, will look into it.

Probably we can rename the current `parse` to something like `parse_ignoring_enabled_state`, and have `parse` use it then check allowed. For the case you mentioned, maybe we can have a `parse_for_all_content` in addition.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4fa69beec8
Make will-change honor prefs properly, and clean it up while at it. r=xidorn

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/24447bd95fbd
followup: set the rule type in the custom properties code, since we use it. r=me on a CLOSED TREE

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ff4fa69beec8
https://hg.mozilla.org/mozilla-central/rev/24447bd95fbd

Comment 9

[Daniel Holbert [:dholbert]]

I was about to post a patch to remove the CSS Containment pref entirely (in bug 1626458), but I noticed this bug's reftest[1] depends on toggling that pref to false[2] (to be sure will-change still works).

If I were to remove the CSS Containment pref, I assume this test would stop testing the codepath/conditions that it's intending to test. So for now, we can't remove that pref. (which is sort of fine, but perhaps not the best thing long-term)

How should we address this moving forward? Should we keep CSS Containment preffable, for the sake of this reftest & its code-coverage? Should we add a dummy pref that we can use for tests like this? Or just get rid of the test and cross our fingers that it doesn't regress? (probably not that one :) ) Something else?

[1] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/layout/reftests/bugs/1466008.html
[2] https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/layout/reftests/bugs/reftest.list#2054

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Just switch it to another pref-controlled property?

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Yeah, so, that reftest relies on the observable effects of contain creating a fixed-pos containing block.

Other properties which are preffed with the same effect are the individual transform properties (layout.css.individual-transform.enabled), or backdrop-filter (layout.css.backdrop-filter.enabled).

Using any of those instead would do.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Yeah, so, that reftest relies on the observable effects of contain creating a fixed-pos containing block.

Other properties which are preffed with the same effect are the individual transform properties (layout.css.individual-transform.enabled)

This pref shipped as default-enabled in Firefox 72 (bug 1424900), and now we're up to having released Firefox 74, so it's kind of in the same boat as contain in the sense that we're unlikely to un-ship it & we might as well remove its pref if possible. So, I'm hesitant to just kick the can over to this property.

or backdrop-filter (layout.css.backdrop-filter.enabled).

That one might be a good fit, since it's complicated & will likely be webrender-dependent for the foreseeable future & hence we'll probably want to keep it preffable for a while.

Question: is it OK that the pref is off by default? (i.e. were you trying to exercise some condition where a default-enabled-property is dynamically preffed off, or is it sufficient for the property to simply be disabled, period [regardless of whether it's default-disabled or dynamically disabled]?)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

It's ok for it to be just disabled.