Bug 177416 - CSS constant properties should not be enabled by default
Summary: CSS constant properties should not be enabled by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-24 08:09 PDT by Michael Catanzaro
Modified: 2017-10-10 14:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.12 KB, patch)
2017-09-24 08:18 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2017-09-24 08:09:19 PDT
CSS constant properties is still experimental, so it shouldn't be enabled by default except for Cocoa ports.
Comment 1 Michael Catanzaro 2017-09-24 08:18:13 PDT
Hm I found r219318. This should probably be reviewed by Dean. Dean, we really do not want any experimental features enabled by default in GTK and WPE releases, only for Safari Tech Preview, which is what the DEFAULT_EXPERIMENTAL_FEATURES_ENABLED setting is trying to accomplish. If CSS constant properties is ready for Safari releases, then it should no longer be in the experimental features list.

I know there's been some desire for a runtime-enumerable list of feature toggles, but I don't think we should be abusing the experimental features list for that purpose.
Comment 2 Michael Catanzaro 2017-09-24 08:18:39 PDT
Created attachment 321653 [details]
Patch
Comment 3 Darin Adler 2017-09-24 17:03:44 PDT
Comment on attachment 321653 [details]
Patch

I am not sure this is right for Safari Developer Preview. Does someone at Apple have to do something to turn it on by default for that if we turn it off here?
Comment 4 Darin Adler 2017-09-24 17:04:36 PDT
Comment on attachment 321653 [details]
Patch

Oh, I see. DEFAULT_EXPERIMENTAL_FEATURES_ENABLED does mean "on for developer preview". I wonder if it’s right for actual iOS and macOS releases too. I would say review+ if I was more of an expert on how this works.
Comment 5 Michael Catanzaro 2017-09-24 19:35:36 PDT
(In reply to Darin Adler from comment #4)
> Comment on attachment 321653 [details]
> Patch
> 
> Oh, I see. DEFAULT_EXPERIMENTAL_FEATURES_ENABLED does mean "on for developer
> preview". I wonder if it’s right for actual iOS and macOS releases too. I
> would say review+ if I was more of an expert on how this works.

Because Apple doesn't have ENABLE(DEVELOPER_MODE) or ENABLE(EXPERIMENTAL_FEATURES) (which is introduced in bug #177338), currently for macOS and iOS the experimental features are disabled manually after branching. So this change doesn't affect Apple at all, only WPE/GTK and WinCairo.

This process seems confusing and non-ideal to me, but if it's what works best for your release managers then I don't see any need to change it.
Comment 6 Michael Catanzaro 2017-09-24 19:41:10 PDT
DEFAULT_EXPERIMENTAL_FEATURES_ENABLED probably needs to be renamed, by the way. Originally I envisioned that all experimental features would be either enabled or disabled, but I didn't realize that Safari Tech Preview would want to enable some but not all experimental features. So there are actually two valid options for experimental features: always disabled by default (for the really unstable stuff), and enabled only for tech previews and GTK/WPE developer builds (DEFAULT_EXPERIMENTAL_FEATURES_ENABLED).
Comment 7 Michael Catanzaro 2017-10-04 00:42:22 PDT
Seems this got forgotten. Darin, can you give r+ please? Since this doesn't affect Apple and it would be good to get it into our next GTK release.
Comment 8 WebKit Commit Bot 2017-10-10 14:24:18 PDT
Comment on attachment 321653 [details]
Patch

Clearing flags on attachment: 321653

Committed r223143: <http://trac.webkit.org/changeset/223143>
Comment 9 WebKit Commit Bot 2017-10-10 14:24:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-10-10 14:25:36 PDT
<rdar://problem/34919586>