1267890 - Support detecting bool preferences in chrome stylesheets

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

In bug 1259889, we added support for putting rules in UA sheets behind bool prefs. We also want this to be available for chrome stylesheets.

Bug 1259889 uses @supports to implement this, which completes in parse-time, which is probably preferable given that in most of time, we only parse every UA sheet once in the browser's lifetime, and those prefs aren't expected to be changed frequently.

But that doesn't really fit requirements of chrome sheets. Prefs used in chrome could be changed frequently, and chrome may want to respond to the changes quickly. Reparsing sheets isn't usually easy. So we may want to also support this feature via media query.

But making media query performant could be challenging in this case. The wip patch in bug 677302 tracks all pref change if such media query is found. I suspect whether it is a good idea.

Comment 1

[Marat Tanalin]

Is it supposed to cover stylesheets in extensions? Thanks.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Marat Tanalin | tanalin.com from comment #1)
> Is it supposed to cover stylesheets in extensions? Thanks.

I think so as far as stylesheets in extensions have chrome privilege.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Would this be useful in rolling out the Photon effort? Should we mark this as a platform dependency?

Comment 5

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #4)
> Would this be useful in rolling out the Photon effort? Should we mark this
> as a platform dependency?

For the visual refresh I was planning on using a build-time switch. @media might be preferable but maybe not worth the effort since it's a temporary setup. However, it seems like this would be really useful for other (also photon related) work such as bug 1351413.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #5)
> However, it seems like this would be really useful for other (also
> photon related) work such as bug 1351413.

I meant bug 1352069.

Comment 7

[Jim Porter (:squib)]

(In reply to Mike Conley (:mconley) from comment #4)
> Would this be useful in rolling out the Photon effort? Should we mark this
> as a platform dependency?

It would be really nice to have this for Photon animations, which users will be allowed to disable even after Photon has rolled out.

Comment 8

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

Some of the work done in bug 677302 may still be useful here.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

It seems we didn't port the implementation in bug 1259889 to stylo (since we are no longer using that anymore, and we don't really have a test for it), so we have a chance to redesign bool pref in stylesheet.

The first question would be whether we really need the stylesheet to respond to dynamic change of pref. If we don't, I would suggest that implementing this as part of @supports like what we did in bug 1259889 would be the simplest and probably the most performant approach.

If we need dynamic change, then we would need something similar to @media rule. The work in bug 677302 makes me concerned about the performance, because that would require pref lookup for each pref usage in stylesheet every time when any media feature value changes. Pref lookup is a hash table lookup from string, which may not be as cheap as we want in restyling, so maybe we would need to implement some kind of cache.

After thinking about cache, I guess the whole thing may not be that complicated as I thought before. It would still be some more code, though.

Comment 10

[Brian Grinstead [:bgrins]]

FWIW at least for the flexbox emulation case I think not responding to dynamic pref changes is OK. And I am assuming the layout changes to do the emulation may require a restart anyway (needinfo :dholbert to confirm).

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> FWIW at least for the flexbox emulation case I think not responding to
> dynamic pref changes is OK.

To elaborate, the initial target for that work would be to get the emulated flexbox mode set up in taskcluster so we can compare screenshots / perf / etc over time. And for that case I'm sure we can set up a pref in the profile ahead of startup.

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> I am assuming the layout changes to do the
> emulation may require a restart anyway (needinfo :dholbert to confirm).

I'm pretty sure you're correct, yes.  (The layout changes would need the equivalent of a web page reload to take effect on existing elements; and for frontend code, I think that effectively means a browser restart.)

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1267890 part 1 - Add @supports -moz-bool-pref() support for stylo.

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

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1267890 part 2 - Add test for -moz-bool-pref().

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

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

The syntax would be the same as before:
```
@supports -moz-bool-pref("pref.name") {
}
```
and it can also be combined with the boolean operators of @supports e.g. `@supports not -moz-bool-pref("pref.name")`.

Note that, each use of that would trigger a pref query at parse time. Chrome stylesheets are cached after parsing, so this is a one-time query on the whole lifetime of a process, but it may still have some cost at startup.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935662 [details]
Bug 1267890 part 1 - Add @supports -moz-bool-pref() support for stylo.

https://reviewboard.mozilla.org/r/206554/#review212278

Codewise looks good, with the nits addressed.

Just to be clear, from the comments it seems we're not intending to handle any sort of dynamic change to this gracefully, are we?

If so, please mention it in the docs of the `MozBoolPref` variable. Otherwise, it'd be better for this to be an `@media` query, since we have infra for those already.

::: layout/style/ServoBindings.h:718
(Diff revision 1)
>  
>  const nsTArray<mozilla::dom::Element*>* Gecko_GetElementsWithId(
>      const nsIDocument* aDocument,
>      nsAtom* aId);
>  
> +// Check the value of the given bool preference.

nit: Mention that the pref name needs to be null-terminated?

::: servo/components/style/stylesheets/supports_rule.rs:157
(Diff revision 1)
>                  }
>              }
> -            Token::Function(_) => {}
> +            Token::Function(ident) => {
> +                // Although this is an internal syntax, it is not necessary to check
> +                // parsing context as far as we accept any unexpected token as future
> +                // syntax, and evaluate it to false when not in chrome / ua sheet.

nit: Could you link to the link pointing out to the future syntax stuff?

::: servo/components/style/stylesheets/supports_rule.rs:158
(Diff revision 1)
>              }
> -            Token::Function(_) => {}
> +            Token::Function(ident) => {
> +                // Although this is an internal syntax, it is not necessary to check
> +                // parsing context as far as we accept any unexpected token as future
> +                // syntax, and evaluate it to false when not in chrome / ua sheet.
> +                if ident == "-moz-bool-pref" {

nit: You probably want `eq_ignore_ascii_case`, for consistency with every other CSS function.

::: servo/components/style/stylesheets/supports_rule.rs:162
(Diff revision 1)
> +                // syntax, and evaluate it to false when not in chrome / ua sheet.
> +                if ident == "-moz-bool-pref" {
> +                    if let Ok(name) = input.try(|i| {
> +                        i.parse_nested_block(|i| {
> +                            i.expect_string()
> +                                .map(|s| s.to_string())

nit: I think you can rejigger this to avoid the extra `to_string` allocation.

::: servo/components/style/stylesheets/supports_rule.rs:163
(Diff revision 1)
> +                if ident == "-moz-bool-pref" {
> +                    if let Ok(name) = input.try(|i| {
> +                        i.parse_nested_block(|i| {
> +                            i.expect_string()
> +                                .map(|s| s.to_string())
> +                                .map_err(CssParseError::<()>::from)

Huh, I'd expect you wouldn't need this `From::from`... oh well.

::: servo/components/style/stylesheets/supports_rule.rs:196
(Diff revision 1)
>  }
>  
> +#[cfg(feature = "gecko")]
> +fn eval_moz_bool_pref(name: &CStr, cx: &ParserContext) -> bool {
> +    use gecko_bindings::bindings;
> +    if cx.stylesheet_origin != Origin::UserAgent && !cx.url_data.is_chrome() {

nit: `chrome_rules_enabled` instead of `url_data.is_chrome()` if you want it also exposed to user sheets and such.

::: servo/components/style/stylesheets/supports_rule.rs:261
(Diff revision 1)
>                  decl.to_css(dest)?;
>                  dest.write_str(")")
>              }
> +            SupportsCondition::MozBoolPref(ref name) => {
> +                dest.write_str("-moz-bool-pref(")?;
> +                let name = str::from_utf8(name.as_bytes())

Huh, this linear conversion is slightly unfortunate, but not that it matters, really, I guess.

::: servo/components/style/stylesheets/supports_rule.rs:263
(Diff revision 1)
>              }
> +            SupportsCondition::MozBoolPref(ref name) => {
> +                dest.write_str("-moz-bool-pref(")?;
> +                let name = str::from_utf8(name.as_bytes())
> +                    .expect("Should be parsed from valid UTF-8");
> +                serialize_string(name, dest)?;

nit: You can just use `name.to_css(dest)`

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935663 [details]
Bug 1267890 part 2 - Add test for -moz-bool-pref().

https://reviewboard.mozilla.org/r/206562/#review212280

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #16)
> Comment on attachment 8935662 [details]
> Bug 1267890 part 1 - Add @supports -moz-bool-pref() support for stylo.
> 
> https://reviewboard.mozilla.org/r/206554/#review212278
> 
> Codewise looks good, with the nits addressed.
> 
> Just to be clear, from the comments it seems we're not intending to handle
> any sort of dynamic change to this gracefully, are we?

That's my question in comment 9, and from the comments after that, it seems people are fine without dynamic change.

I'm not sure whether bug 1362128 wants dynamic change.

> If so, please mention it in the docs of the `MozBoolPref` variable.
> Otherwise, it'd be better for this to be an `@media` query, since we have
> infra for those already.

We would need much more code than this for handling dynamic change, I suppose, since I don't quite believe we want to query pref every time we invalidate media query.

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8935662 [details]
Bug 1267890 part 1 - Add @supports -moz-bool-pref() support for stylo.

https://reviewboard.mozilla.org/r/206554/#review212278

> nit: I think you can rejigger this to avoid the extra `to_string` allocation.

I'm not sure how can I avoid any allocation here. `expect_string` returns a `CowRcStr`, from which you cannot extract a `String` without reallocation, and `CString` wants a `Into<Vec<u8>>`, so there needs to be a owned object passed in. I don't see there's anything I can do better here.

> Huh, I'd expect you wouldn't need this `From::from`... oh well.

It doesn't seem to work, otherwise I wouldn't add it.

> nit: `chrome_rules_enabled` instead of `url_data.is_chrome()` if you want it also exposed to user sheets and such.

I considered this... and actually I'm fine with either way. Probably giving user sheets more power is also useful, so ok, let's use `chrome_rules_enabled` instead.

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8935662 [details]
Bug 1267890 part 1 - Add @supports -moz-bool-pref() support for stylo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206554/diff/1-2/

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8935663 [details]
Bug 1267890 part 2 - Add test for -moz-bool-pref().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/206562/diff/1-2/

Comment 22

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 4d13824f3af3 -d 33d7b8a51e80: rebasing 438745:4d13824f3af3 "Bug 1267890 part 1 - Add @supports -moz-bool-pref() support for stylo. r=emilio"
merging servo/components/style/stylesheets/supports_rule.rs
warning: conflicts while merging servo/components/style/stylesheets/supports_rule.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95d632802624
part 1 - Add @supports -moz-bool-pref() support for stylo. r=emilio
https://hg.mozilla.org/integration/autoland/rev/2eeea549cdfe
part 2 - Add test for -moz-bool-pref(). r=emilio

Comment 24

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

Is this done in a way that it guarantees we never make a synchronous pref query to a different process during stylesheet parsing?

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #24)
> Is this done in a way that it guarantees we never make a synchronous pref
> query to a different process during stylesheet parsing?

Since I don't know how to guarantee that, so probably the answer is no.

I guess if people want to use it in content process, they should probably add the pref to the startup list.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Servo PR

Comment 27

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

(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive until Austin) from comment #25)
> (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #24)
> > Is this done in a way that it guarantees we never make a synchronous pref
> > query to a different process during stylesheet parsing?
> 
> Since I don't know how to guarantee that, so probably the answer is no.

I guess one way would be to make the entire feature parent-process-only.  Even if not, it seems like we ought to be able to guarantee it, or at least assert it in debug builds, though.  (It shouldn't be too hard to assert that a pref is in mozilla::dom::ContentPrefs::gEarlyPrefs.

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #27)
> (In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive until Austin) from
> comment #25)
> > (In reply to David Baron :dbaron: ⌚️UTC-8 from comment #24)
> > > Is this done in a way that it guarantees we never make a synchronous pref
> > > query to a different process during stylesheet parsing?
> > 
> > Since I don't know how to guarantee that, so probably the answer is no.
> 
> I guess one way would be to make the entire feature parent-process-only. 
> Even if not, it seems like we ought to be able to guarantee it, or at least
> assert it in debug builds, though.  (It shouldn't be too hard to assert that
> a pref is in mozilla::dom::ContentPrefs::gEarlyPrefs.

Although neither of them is hard, both of them would break the reftest.

I'm now wondering whether it really needs to be in early prefs at all.

It seems to me that all prefs should be setup inside ContentChild::InitXPCOM (which calls Preferences::SetLatePreferences) [1]. This method also calls nsLayoutStylesheetCache::SetUserContentCSSURL to store user content sheet URL [2]. In nsLayoutStylesheetCache, the value set here is used only once in its constructor [3] which indicates that all preferences should have been ready even before we start parsing our UA sheets. (Otherwise, either userContent.css should have been broken, or that piece of code is useless at all.)

(That piece of code does look weird, though. Shouldn't we load userContent.css in InitFromProfile? If we don't, we probably should skip that when in content process. Bug 1358524 does seem to indicate that the later loading code is effective, anyway.)

[1] https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/dom/ipc/ContentChild.cpp#1153
[2] https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/dom/ipc/ContentChild.cpp#1222-1224
[3] https://searchfox.org/mozilla-central/rev/fd0b701b6f4bce8dead150c569dd68fb9f4c7ed9/layout/style/nsLayoutStylesheetCache.cpp#358-365

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da5f3201fc92
followup - Enable -moz-bool-pref in chrome sheets in Gecko as well.

Comment 30

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/95d632802624
https://hg.mozilla.org/mozilla-central/rev/2eeea549cdfe
https://hg.mozilla.org/mozilla-central/rev/da5f3201fc92