1349949 - Allow having a single pref flip turn webrender on or off

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

Now that we have webrender building by default (in bug 1342450), we want to be able to turn it on and off with a single pref flip. That's almost happening already, but in all.js there are some additional advanced layers prefs that also get flipped by the MOZ_ENABLE_WEBRENDER define [1]. That means if a user builds with --enable-webrender=build, and wants to turn on webrender to behave the same as it would with --enable-webrender=true, they would have to manually flip gfx.webrender.enabled and also flip the necessary advanced layers prefs.

Note that this affects not just the user flow, but also the automation flow, because otherwise to implement bug 1342488 we will need to flip a bunch of prefs instead of just one.

I have an idea for making those advanced layers prefs conditional on webrender being enabled so that we don't have to enumerate all of these prefs separately.

[1] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/modules/libpref/init/all.js#5648

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=13984d568ca5a4d634b822de02e9b77f6249f5a8 (with my patches rebased onto the graphics branch). But I intend to land the patches on inbound/autoland after the next m-c -> graphics merge.

/cc mchang and ethlin, please take a look at the patches in the try push above (particularly the second one) as I think you guys are the ones most likely to be adding new prefs of this sort in the near future.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1349949 - Add the notion of an "override pref" to gfxPrefs.h.

An override pref is one that is stored as a tri-state int32_t pref in the gecko
preferences system (with values 0, 1, or 2), but is exposed as a bool via the
gfxPrefs API. The int32_t value defaults to 2, which means that gfxPrefs will
take the value from somewhere else (which can be customized per-pref). If the
int32_t value stores 0 or 1, those correspond to force-disabled and force-enabled
respectively, and the gfxPrefs API will return false or true.

This allows the default value of a pref to be conditional upon other features
(e.g. webrender being enabled or not), but still allows the user or code to
override it into the enabled or disabled state.

Review commit: https://reviewboard.mozilla.org/r/123188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123188/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1349949 - Turn some advanced layers prefs into override prefs.

This allows us to remove the #ifdef MOZ_ENABLE_WEBRENDER for the advanced layers
prefs from all.js. As additional advanced layers are turned on for webrender (or
non-webrender) they can be converted into override prefs without affecting the
call sites.

Review commit: https://reviewboard.mozilla.org/r/123190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123190/

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> But I intend to land the patches
> on inbound/autoland after the next m-c -> graphics merge.

This should say graphics -> m-c merge. Which happened earlier today. So now graphics and m-c are pretty close. I'll land these patches on m-c and merge them back to graphics the next day. As part of the merge I'll need to fix up any other prefs that landed in the meantime (Mason already landed layers.advanced.boxshadow-inset-layers which I'll have to fix up, for example).

Comment 5

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8850641 [details]
Bug 1349949 - Add the notion of an "override pref" to gfxPrefs.h.

https://reviewboard.mozilla.org/r/123188/#review125600

::: gfx/thebes/gfxPrefs.h:85
(Diff revision 1)
> +// - A value of 0 means that it has been force-disabled, and is exposed as a
> +//   false-valued bool.
> +// - A value of 1 means that it has been force-enabled, and is exposed as a
> +//   true-valued bool.
> +// - A value of 2 (the default) means that it returns the value of a different
> +//   function (which is evaluated to a boolean).

nit: returns the base value as a bool

Comment 6

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8850642 [details]
Bug 1349949 - Turn some advanced layers prefs into override prefs.

https://reviewboard.mozilla.org/r/123190/#review125606

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Thanks. Updated patches to address nit. Turns out the graphics -> m-c merge has not yet propagated to autoland, so I need to wait for that before I can push this.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8850641 [details]
Bug 1349949 - Add the notion of an "override pref" to gfxPrefs.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123188/diff/1-2/

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8850642 [details]
Bug 1349949 - Turn some advanced layers prefs into override prefs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123190/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/617ef3603a8e
Add the notion of an "override pref" to gfxPrefs.h. r=mchang
https://hg.mozilla.org/integration/autoland/rev/cc8baab97e60
Turn some advanced layers prefs into override prefs. r=mchang

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/617ef3603a8e
https://hg.mozilla.org/mozilla-central/rev/cc8baab97e60

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/db0dbb918eeb
Convert another pref to an override pref as part of merging m-c to graphics. r=me

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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