Closed
Bug 1364863
Opened 7 years ago
Closed 7 years ago
Don't force stylo reftest sandbox attribute at build-time
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
(deleted),
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6eadabeec4e7f8e0c51cbab54459bca1d401c5fb
This solves the 'some reftests seem to fail' issue in bug 1356991 comment 11.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8867655 -
Flags: review?(cam)
Comment 2•7 years ago
|
||
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)?
Assignee | ||
Comment 3•7 years ago
|
||
(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•7 years ago
|
||
(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.
Updated•7 years ago
|
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
Comment 6•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•