Closed Bug 1364863 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(1 file)

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)?
(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)?
(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.
Attachment #8867655 - Flags: review?(cam) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/acae4954e802
Make stylo property of reftests dependent on the pref, not just the define. r=heycam
https://hg.mozilla.org/mozilla-central/rev/acae4954e802
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.