1189922 - Add a pref to enable the CSSUnprefixingService globally

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Mike Taylor [:miketaylr]]

To allow for testing on Nightly, and to get a better understanding of what might break with aliasing/unprefixing of certain webkit prefixes, let's add a pref that will enable the unprefixing service for all domains.

Once Bug 1177263 is fixed (however that actually happens), this can be removed.

Comment 1

[Daniel Holbert [:dholbert]]

Thanks, Mike.

BTW - the new logic in ShouldUseUnprefixingService() (in nsCSSParser.cpp) should probably be #ifdef NIGHTLY_BUILD, so that this pref only ever does stuff in Nightly / mozilla-central builds.

Comment 2

[Mike Taylor [:miketaylr]]

Agreed, will update my patch and upload in a bit. Thx.

Comment 3

[mitaylor]

Attachment: . Add a preference to enable global whitelisting of the CSSUnprefixingService. r=dholbert

This is a temporary preference to allow for testing-in-the-wild by a larger
audience, for nightly builds. When 1177263 is fixed this can be removed.

Comment 4

[Mike Taylor [:miketaylr]]

Comment on attachment 8641940 [details] [diff] [review]
. Add a preference to enable global whitelisting of the CSSUnprefixingService. r=dholbert

(first time using git-bz, let me try that again)

Comment 5

[Mike Taylor [:miketaylr]]

Attachment: . Add a preference to enable global whitelisting of the CSSUnprefixingService. r=dholbert

This is a temporary preference to allow for testing-in-the-wild by a larger
audience, for nightly builds. When 1177263 is fixed this can be removed.

Comment 6

[Mike Taylor [:miketaylr]]

Comment on attachment 8641944 [details] [diff] [review]
. Add a preference to enable global whitelisting of the CSSUnprefixingService. r=dholbert

Daniel, should we wrap everything else in NIGHTLY_BUILD ifdefs? Or is it OK to have pref that does nothing everywhere else?

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8641944 [details] [diff] [review]
. Add a preference to enable global whitelisting of the CSSUnprefixingService. r=dholbert

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

> Daniel, should we wrap everything else in NIGHTLY_BUILD ifdefs? Or is it OK
> to have pref that does nothing everywhere else?

Yeah... It's pretty cheap so not a huge deal, but we probably should wrap the sUnprefixingServiceGloballyWhitelisted decl & AddBoolVarCache call in #ifdef NIGHTLY_BUILD, since they're useless in release builds.

r=me with those #ifdefs, and with a few comment nits noted below -- tl;dr: we should avoid language saying that this pref "enables" anything, to avoid confusion about how this pref interacts with the other one (which is actually about "enabling").


Thanks for the patch!

::: layout/style/nsCSSParser.cpp
@@ +6667,5 @@
>      return false;
>    }
> +#ifdef NIGHTLY_BUILD
> +  if (sUnprefixingServiceGloballyWhitelisted) {
> +    // Unprefixing is globally enabled.

s/globally enabled/globally whitelisted, so no need to check mSheetPrincipal/

::: modules/libpref/init/all.js
@@ +2291,5 @@
>  
>  // Is the CSS Unprefixing Service enabled? (This service emulates support
>  // for certain vendor-prefixed properties & values, for sites on a "fixlist".)
>  pref("layout.css.unprefixing-service.enabled", true);
> +// Is the CSS Unprefixing Service enabled for all domains, all the time?

To avoid confusion about how this interacts with the other pref:

s/enabled/whitelisted/
s/all the time//

And also probably worth adding a parenthetical here saying "(Only honored in Nightly builds)" or something like that.

Comment 8

[Mike Taylor [:miketaylr]]

Agreed on the comment improvements. Will update and throw at try later tonight. Thanks for the review!

Comment 9

[Mike Taylor [:miketaylr]]

Attachment: 1189922.-Add-a-preference-to-enable-global-white.patch

Put everything else behind #ifdef NIGHTLY_BUILD and fixed comments according to dholbert's feedback. Carrying forward r+.

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

Comment 10

[Mike Taylor [:miketaylr]]

Try look good. → checkin-needed

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8641979 [details] [diff] [review]
1189922.-Add-a-preference-to-enable-global-white.patch

>+#ifdef NIGHTLY_BUILD
>+  if (sUnprefixingServiceGloballyWhitelisted) {
>+    // Unprefixing is globally globally whitelisted,

nit: "globally globally" wants to just be "globally"

Fixed locally & pushed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/523ffd5c4588

Comment 12

[Mike Taylor [:miketaylr]]

> "globally globally" wants to just be "globally"

Indeed it does. Thanks for catching that!

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/523ffd5c4588