Closed Bug 1411532 Opened 2 years ago Closed 2 years ago

stylo: Add a pref for using stylo on chrome

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

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.
Assignee: nobody → xidorn+moz
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.
Attachment #8922652 - Flags: review?(bzbarsky) → review+
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"
Attachment #8922653 - Flags: review?(bzbarsky) → review+
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...
Pushed by xquan@mozilla.com:
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
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)
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1398996
You need to log in before you can comment on or make changes to this bug.