1364863 - Don't force stylo reftest sandbox attribute at build-time

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eadabeec4e7f8e0c51cbab54459bca1d401c5fb

This solves the 'some reftests seem to fail' issue in bug 1356991 comment 11.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Make stylo property of reftests dependent on the pref, not just the define. v1

Comment 2

[Cameron McCormack (:heycam)]

Comment on attachment 8867655 [details] [diff] [review]
Make stylo property of reftests dependent on the pref, not just the define. v1

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

::: layout/tools/reftest/reftest.jsm
@@ +718,2 @@
>  #ifdef MOZ_STYLO
> +    prefs.getBoolPref("layout.css.servo.enabled", false);

This feels a bit strange to me, as it's overloading what the pref means.  The pref's normal effect is "if set at document load time, selects which style backend to use".  Here it's being used to mean "if set at reftest run start time, set a sandbox variable to true, which we interpret as 'are we running in stylo-vs-gecko mode'".

Should this actually be getting the value of "reftest.compareStyloToGecko" (which maybe we've already initialized gCompareStyloToGecko to, depending on when these initialization bits happen, which I haven't checked)?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Cameron McCormack (:heycam) from comment #2)
> Comment on attachment 8867655 [details] [diff] [review]
> Make stylo property of reftests dependent on the pref, not just the define.
> v1
> 
> Review of attachment 8867655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/tools/reftest/reftest.jsm
> @@ +718,2 @@
> >  #ifdef MOZ_STYLO
> > +    prefs.getBoolPref("layout.css.servo.enabled", false);
> 
> This feels a bit strange to me, as it's overloading what the pref means. 
> The pref's normal effect is "if set at document load time, selects which
> style backend to use".  Here it's being used to mean "if set at reftest run
> start time, set a sandbox variable to true, which we interpret as 'are we
> running in stylo-vs-gecko mode'".

Is that actually the case? I thought that the sandbox property was used to evaluates the fails-if(stylo) stuff (though I haven't checked).

Basically, given the lack of any way to annotate the failures in the reftest manifests, we more or less don't support testing stylo in non-gecko-vs-stylo mode. At some point we may turn off gecko-vs-stylo mode, in which case we want fails-if(stylo) to start referring to the regular reftest behavior.

So I think we really do want to ask "is stylo enabled" rather than "are we doing gecko-vs-servo".

> 
> Should this actually be getting the value of "reftest.compareStyloToGecko"
> (which maybe we've already initialized gCompareStyloToGecko to, depending on
> when these initialization bits happen, which I haven't checked)?

Comment 4

[Cameron McCormack (:heycam)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> > This feels a bit strange to me, as it's overloading what the pref means. 
> > The pref's normal effect is "if set at document load time, selects which
> > style backend to use".  Here it's being used to mean "if set at reftest run
> > start time, set a sandbox variable to true, which we interpret as 'are we
> > running in stylo-vs-gecko mode'".
> 
> Is that actually the case? I thought that the sandbox property was used to
> evaluates the fails-if(stylo) stuff (though I haven't checked).

Yes, that's what the sandbox property is used for.

> Basically, given the lack of any way to annotate the failures in the reftest
> manifests, we more or less don't support testing stylo in non-gecko-vs-stylo
> mode. At some point we may turn off gecko-vs-stylo mode, in which case we
> want fails-if(stylo) to start referring to the regular reftest behavior.

I agree with that.

> So I think we really do want to ask "is stylo enabled" rather than "are we
> doing gecko-vs-servo".

I guess what feels odd to me about checking that pref is that we then later (in the gecko-vs-stylo mode) go about flipping that pref off and on, and so that pref's value doesn't feel like a static description of the environment we're running in (which the "is stylo enabled" question sounds like).  But, in lieu of some different arrangement (like the layout.css.servo.enabled pref being read once at startup, and having a different pref to override that when loading the test and references in gecko-vs-stylo mode), I can accept it.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acae4954e802
Make stylo property of reftests dependent on the pref, not just the define. r=heycam

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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