Closed Bug 1564870 Opened 9 months ago Closed 9 months ago

Add --no-enable-webrender flag

Categories

(Testing :: web-platform-tests, task)

Version 3
task
Not set

Tracking

(firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: jgraham, Assigned: jgraham)

References

Details

Attachments

(1 file)

We are trying to maintain the convention (not always followed historically) that feature flags like --enable-webrender are accompanied by a --no-<feature> flag so that we are able to be explicit in the face of changing defaults, and can write wrapper scripts that override feature settings.

For this reason add a --no-enable-webrender flag that forces WebRender to be disabled.

This allows us to be more explicit about when the feature is enabled or not, rather than
relying on the defaults.

One of the explicit goals of adding --enable-webrender was to ensure that WR gets enabled if and only if that flag is provided. So from my point of view, adding a --no-enable-webrender undermines that a little bit because it is a little less clear what happens if neither flag is provided. That being said I don't have strong objections if you want to add the --no flag, although I would prefer an implementation that simply leaves the existing code as-is and errors out if both flags are provided.

I have the opposite reaction; something that makes the absence of a flag enforce behaviour that's different from the shipping product is confusing because the standard assumption is that the no-flag case is just like what a consumer would use, except for changes that make the product more testable (e.g. disabling telemetry). This change allows people to be explicit in all cases where they care and I would quite strongly prefer that in our task definitions we actually always supplied either --enable-webrender or --no-enable-webrender so that it's obvious that changing product defaults doesn't have any effect there.

Having on/off flags also allows us to change the defaults in various environments, and we already use this capability with other flags e.g. upstream wpt runs tests on local machines with headless on be default since that's more user friendly, but in CI and m-c still defaults to the non-headless mode because that's more correct.

Erroring if multiple options are supplied would make sense but it's different to the way existing options work, and requires some hackery to work with argparse (e.g. multiple option names and custom code to resolve them, or a list of all values and custom code to error if the length is > 1 or something). It also forbids some use cases like wrapper scripts that override the default set by an inner wrapper script.

(In reply to James Graham [:jgraham] [Away until 2019-07-08] from comment #3)

the standard assumption is that the no-flag case is just like what a consumer would use

For WebRender the conditions that enable it by default are super complex, involving things like whether you're on a laptop vs desktop, your screen size, graphics card, etc. For this reason we basically never want any test harnesses to fall back to the case where the product "default" behaviour is chosen; we want to make sure WR is always either explicitly enabled or disabled.

I would quite strongly prefer that in our task definitions we actually always supplied either --enable-webrender or --no-enable-webrender so that it's obvious that changing product defaults doesn't have any effect there.

As a general principle I agree with this. In this scenario though I felt that making the --no-enable-webrender explicitly implied by lack of --enable-webrender would be better because it leaves only two possibilities (providing the flag or not) vs 4 possibilities, two of which (neither flag provided, or both flags provided) are undesirable.

Like I said, I don't have strong objections to your adding this flag, as long as the expected usage is clear, and we retain the behaviour that absence of --enable-webrender will explicitly force WR off instead of falling back to the "product default" behaviour (which is too complex to be a sane default for anybody).

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/76af8bbce61a
Add --no-enable-webrender flag to wpt, r=maja_zf
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/17787 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.