1411532 - stylo: Add a pref for using stylo on chrome

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Xidorn Quan [:xidorn] UTC+11]

I didn't think this is useful, but I changed my mind, and now think it would still be nice to have a pref that we can experiment with and use as an emergency switch.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1411532 part 1 - Add a pref for enabling stylo on chrome documents.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1411532 part 2 - Add a line to about:support that says whether stylo is enabled for chrome.

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922652 [details]
Bug 1411532 part 1 - Add a pref for enabling stylo on chrome documents.

https://reviewboard.mozilla.org/r/193746/#review199110

::: dom/ipc/ContentPrefs.cpp:125
(Diff revision 1)
>    "javascript.options.wasm_ionjit",
>    "javascript.options.werror",
>    "javascript.use_us_english_locale",
>    "jsloader.shareGlobal",
>  #ifdef MOZ_STYLO
> +  "layout.css.servo.chrome.enabled",

This is the list of prefs that we need available in the content process before the first IPC message is received in said content process.  Do we actually create either an nsDocument or a XUL prototype element in the child process before this happens?

I can live with this as a temporary measure, I guess...

::: layout/base/nsLayoutUtils.h:2545
(Diff revision 1)
> +      // Otherwise we only use stylo on non-e10s parent.
> +      return !XRE_IsE10sParentProcess();
> -  }
> +    }
> +#endif
> +    // Stylo is not enabled for any other process.
> +    // XXX maybe we should assert unreachable here? Are we creating any

Please do so assert.  Actually at least diagnostic assert, if not release assert.

::: layout/base/nsLayoutUtils.cpp:8239
(Diff revision 1)
>    }
>  }
> +
> +/* static */
> +bool
> +nsLayoutUtils::StyloChromeEnabled()

This does not allow dynamic toggling of this pref.  If that's purposeful, please document why.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8922653 [details]
Bug 1411532 part 2 - Add a line to about:support that says whether stylo is enabled for chrome.

https://reviewboard.mozilla.org/r/193748/#review199124

::: commit-message-e1bc9:1
(Diff revision 1)
> +Bug 1411532 part 2 - Add info of stylo chrome support to about:support. r?bz

"Add a line to about:support that says whether stylo is enabled for chrome"

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8922652 [details]
Bug 1411532 part 1 - Add a pref for enabling stylo on chrome documents.

https://reviewboard.mozilla.org/r/193746/#review199110

> This is the list of prefs that we need available in the content process before the first IPC message is received in said content process.  Do we actually create either an nsDocument or a XUL prototype element in the child process before this happens?
> 
> I can live with this as a temporary measure, I guess...

We may... actually I don't know, but I thought it is safer to do so since we are going to not allow flipping this option in runtime.

> This does not allow dynamic toggling of this pref.  If that's purposeful, please document why.

Yes, it is purposeful. I added some comment in init/all.js.

Basically, flipping stylo pref in runtime can lead to mix style backend in one page, which would cause issues like bug 1404020, and for XUL it can be even worse because of bug 1408235 (that style attribute doesn't work when XUL prototype element use a different backend than its target document).

The risk still exists, though, that if someone toggles stylo pref itself...

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8922652 [details]
Bug 1411532 part 1 - Add a pref for enabling stylo on chrome documents.

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

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8922653 [details]
Bug 1411532 part 2 - Add a line to about:support that says whether stylo is enabled for chrome.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49750002c7a6
part 1 - Add a pref for enabling stylo on chrome documents. r=bz
https://hg.mozilla.org/integration/autoland/rev/af22292a7122
part 2 - Add a line to about:support that says whether stylo is enabled for chrome. r=bz

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8e5f681ccbb
followup - Fix eslint failures.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/49750002c7a6
https://hg.mozilla.org/mozilla-central/rev/af22292a7122
https://hg.mozilla.org/mozilla-central/rev/f8e5f681ccbb

Comment 11

[Darkspirit]

Nightly 58 x64 20171028100423 de_DE @ Debian Testing
layout.css.servo.chrome.enabled;true <3
> Stylo: content = true (aktiviert (Standard)), chrome = true (aktiviert durch Benutzer)