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

RESOLVED FIXED in Firefox 62

Status

()

enhancement
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: dbaron, Assigned: emilio)

Tracking

unspecified
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

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'.
(Assignee)

Comment 1

11 months ago
Will take some time to clean it up while at it.
Assignee: nobody → emilio
Comment hidden (mozreview-request)

Comment 3

11 months ago
mozreview-review
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)
})
```
Attachment #8982463 - Flags: review?(xidorn+moz) → review+
(Assignee)

Comment 4

11 months ago
mozreview-review-reply
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

11 months ago
mozreview-review-reply
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

11 months ago
Pushed by ecoal95@gmail.com:
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

11 months ago
Pushed by ecoal95@gmail.com:
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
(Assignee)

Updated

11 months ago
Blocks: 1466136

Comment 8

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff4fa69beec8
https://hg.mozilla.org/mozilla-central/rev/24447bd95fbd
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.