864005 - remove the layout.css.supports-rule.enabled pref

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

At some point we should remove the layout.css.supports-rule.enabled pref.  We at least should wait until Firefox 22, which is the first version for the pref to be enabled, is Release.  How many versions after that should we wait?

Comment 1

[Florian Bender]

Maybe target this for Firefox 27 now?

Comment 2

[Daniel Holbert [:dholbert]]

The pref ended up being default-enabled (for release builds) in bug 855455, which landed on trunk over a year ago (and shipped a few months later).

We're probably fine to remove the pref now, I'd expect?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, absolutely.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix: remove usages (except in one autogenerated reftest.list file)

This removes the pref everywhere except for one spot:
 layout/reftests/w3c-css/received/reftest.list

Per the comment at the top of that file, it's auto-generated (by import-tests.py) and should not be human-edited.

That straggling line will go away the next time we run import-tests.py (which this patch modifies to no longer add this pref).  I did a trial run of import-tests.py to make sure that's true. (It does drop the line, and it also modifies various tests (mostly in css3-namespace) which have presumably been changed upstream since the last time we imported. Since those test-changes are outside the scope of this bug, I'm not going to do the import-tests.py run / reftest.list-tweak here.)

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8427320 [details] [diff] [review]
fix: remove usages (except in one autogenerated reftest.list file)

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

Looks good.

::: layout/reftests/w3c-css/import-tests.py
@@ -48,5 @@
>      "column-width"
>  ]
>  
>  gDefaultPreferences = {
> -    "css3-conditional": "pref(layout.css.supports-rule.enabled,true)"

Maybe leave a comment in here with an example so that it's easier for the next person to add in an entry?

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: fix v2 [r=heycam]

(In reply to Cameron McCormack (:heycam) from comment #5)
> >  gDefaultPreferences = {
> > -    "css3-conditional": "pref(layout.css.supports-rule.enabled,true)"
> 
> Maybe leave a comment in here with an example so that it's easier for the
> next person to add in an entry?

Excellent point. I've added the following comment above this now-empty map:

# Map of about:config prefs that need toggling, for a given test subdirectory.
# Entries should look like:
#  "$SUBDIR_NAME": "pref($PREF_NAME, $PREF_VALUE)"
#
# For example, when "@supports" was behind a pref, gDefaultPreferences had:
#  "css3-conditional": "pref(layout.css.supports-rule.enabled,true)"

I verified that the script still runs correctly, too. (and drops the pref() annotation when it's run)

Sanity-check Try run, before landing, to be sure this patch doesn't break reftests/mochitests:
 https://tbpl.mozilla.org/?tree=Try&rev=ec1a9548bb2a

Comment 7

[Daniel Holbert [:dholbert]]

Ah, Try run has reftest failures -- apparently the pref() line in the auto-generated reftest.list file causes trouble when the pref no longer exists.

I guess *technically* the steps here should be:
 (1) remove pref from import-tests.py
 (2) re-run import-tests.py (dropping the pref from reftest.list)
 (3) remove support for the pref everywhere else

However, I'd rather avoid overcomplicating things. Fortunately, the pref()-removal is the *only* change to this reftest.list file, when I run import-tests.py (though there are a few other changes as well, e.g. importing some new wcss3-namespace tests).  So rather than doing a full import-tests.py run in service of this bug, I'm just going to cherrypick that (automated) reftest.list tweak and land it as part of this patch.

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8433882 [details] [diff] [review]
fix v2 [r=heycam]

Since this changes a .webidl file, apparently I need DOM peer review.

khuey, could you sanity-check the .webidl changes? (just removing "Pref=..." annotations)

Comment 9

[Daniel Holbert [:dholbert]]

Handy link-to-the-only-webidl-change-in-the-patch:
https://bugzilla.mozilla.org/attachment.cgi?id=8433882&action=diff#a/dom/webidl/CSS.webidl_sec1

Comment 10

[Daniel Holbert [:dholbert]]

Thanks. Landed (w/ reftest.list tweak, per end of comment 7):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/8137176a3173

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8137176a3173