1290013 - add a pref to disable/enable stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In order to use reftests to verify stylo against stock Gecko rendering, we need a way to turn stylo on or off for a given document.  We can add a method to control whether a docshell will load its next document with stylo enabled.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: patch

Is this patch enough to get reftest stylo mode switching working?

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Rebased.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 4

[Bobby Holley (:bholley)]

Dumb question - why can't we just make it pref-controlled, and toggle the pref back and forth between loads on the reftest? It seems like that would be a lot simpler than munging the docshell, and give us automatic UI as well.

Comment 5

[Cameron McCormack (:heycam)]

That would work too.  And I guess, since we will eventually want this to have a pref when we ship in Nightly, may as well do that now.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: patch v2

I don't think there should be any problems, but Shing can you test this patch with some reftest runner changes that set the pref?

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8778027 [details] [diff] [review]
patch v2

Review of attachment 8778027 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those fixes.

::: dom/base/nsDocument.cpp
@@ +13413,5 @@
>    // we avoid Servo-backed style sets for SVG documents.
>    if (!mDocumentContainer) {
>      NS_WARNING("stylo: No docshell yet, assuming Gecko style system");
> +  } else if (nsLayoutUtils::SupportsServoStyleBackend(this) &&
> +             Preferences::GetBool("layout.stylo.enabled")) {

Can you use a BoolVarCache instead? No sense in doing hash table lookups when we don't need to.

::: modules/libpref/init/all.js
@@ +5489,5 @@
>  #endif
> +
> +#ifdef MOZ_STYLO
> +// Is the Servo-backed style system enabled?
> +pref("layout.stylo.enabled", true);

Nit: Can we call this layout.css.servo.enabled instead? This pref is likely to persist for a few releases when we ship, and I'd rather keep the rather-opaque 'stylo' moniker from propagating too far.

Comment 8

[Bobby Holley (:bholley)]

Oh also - can you move the pref check to nsPresContext::StyloEnabled instead? It seems more appropriate there.

Comment 9

[Cameron McCormack (:heycam)]

I moved it to nsLayoutUtils, where we've got a bunch of other on-Initialize pref caches set up.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba23c008090d
Add a pref to disable stylo. r=bholley

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba23c008090d

Comment 12

[Shing Lyu [:shinglyu]]

Comment on attachment 8778027 [details] [diff] [review]
patch v2

Cleaning up