Experimental and (new) Internal feature flags
I just added support for a new category of WebKit2 preference: internal debug. The motivating factor was that people were abusing experimental features to add flags for anything they wanted to toggle at runtime, even if our users would have no idea what it meant (nor should they ever toggle it). Internal features are not public-facing. My next task will be to remove the ability to set a default value for experimental features. They will default to be off (if it really is experimental, it shouldn't be on by default). This will apply even to tests - see below for how to turn them on at runtime inside WKTR. Documented here: https://trac.webkit.org/wiki/ExperimentalAndInternalFeatureFlags WebKit provides a way to expose a feature flag to a client application at runtime, and have that application toggle the feature. This lets us to start work on experimental features without fully exposing them to the Web, but allows Web developers to turn them on and test. There are two types of exposed features: Experimental and Internal Debug. The configuration file WebPreferences.yaml controls these features. Just set the category value to experimental or internal. Note that you should also provide a nice human-understandable description and name. These are only exposed by WebKit2. If you want your feature to be toggled via WebKit1 you'll have to manually add more code. Experimental Features ---- These features are designed for Web developers, and are exposed via Safari's Develop menu as well as MiniBrowser. Internal Debug Features ---- These features are designed for WebKit developers, and are exposed via Safari's secret-but-not-really Debug menu. We do not expect Web developers to change these settings. They are also exposed by MiniBrowser. Which should I use? ---- If a Web developer wouldn't understand what the feature is by name, or it is just for internal testing, then you should use an internal feature. For example, we don't want users to disable Service Workers, and almost no one would know what MDSN ICE Candidates are. What about testing? ---- You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example... <!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] --> Dean
On Wed, Sep 12, 2018 at 5:48 PM Dean Jackson <dino@apple.com> wrote:
I just added support for a new category of WebKit2 preference: internal debug.
The motivating factor was that people were abusing experimental features to add flags for anything they wanted to toggle at runtime, even if our users would have no idea what it meant (nor should they ever toggle it). Internal features are not public-facing.
My next task will be to remove the ability to set a default value for experimental features. They will default to be off (if it really is experimental, it shouldn't be on by default). This will apply even to tests - see below for how to turn them on at runtime inside WKTR.
Documented here: https://trac.webkit.org/wiki/ExperimentalAndInternalFeatureFlags
WebKit provides a way to expose a feature flag to a client application at runtime, and have that application toggle the feature. This lets us to start work on experimental features without fully exposing them to the Web, but allows Web developers to turn them on and test. There are two types of exposed features: Experimental and Internal Debug.
The configuration file WebPreferences.yaml controls these features. Just set the category value to experimental or internal. Note that you should also provide a nice human-understandable description and name.
These are only exposed by WebKit2. If you want your feature to be toggled via WebKit1 you'll have to manually add more code.
Experimental Features ----
These features are designed for Web developers, and are exposed via Safari's Develop menu as well as MiniBrowser.
Internal Debug Features ----
These features are designed for WebKit developers, and are exposed via Safari's secret-but-not-really Debug menu. We do not expect Web developers to change these settings. They are also exposed by MiniBrowser.
Which should I use? ----
If a Web developer wouldn't understand what the feature is by name, or it is just for internal testing, then you should use an internal feature. For example, we don't want users to disable Service Workers, and almost no one would know what MDSN ICE Candidates are.
What about testing? ----
You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example...
<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] -->
Is there a plan for how to handle web platform tests (where we can't add such a header)? Currently, WebKitTestRunner enables all experimental features in tests (including those disabled by default), so adding an experimental feature (even one that's disabled by default) is a convenient way to get web platform test coverage for the feature. If WebKitTestRunner will no longer do that, we'll need some other way to keep these tests running.
On 13 Sep 2018, at 08:05, Ali Juma <ajuma@chromium.org> wrote:
Is there a plan for how to handle web platform tests (where we can't add such a header)?
Currently, WebKitTestRunner enables all experimental features in tests (including those disabled by default), so adding an experimental feature (even one that's disabled by default) is a convenient way to get web platform test coverage for the feature. If WebKitTestRunner will no longer do that, we'll need some other way to keep these tests running.
I thought about this. I see three options: 1. Continue to enable all experimental features in testing. The downside is that we're testing something different to what we ship. 2. Have the default value only apply to testing, not to shipping. For Internal Features, instead of enabling them all, I had them revert to their default value. 3. Have some way to add the headers after importing the WPT test. For now, I added headers to the WPT code, because there already were a bunch of tests that had such headers. Dean
On Wed, Sep 12, 2018 at 3:09 PM Dean Jackson <dino@apple.com> wrote:
On 13 Sep 2018, at 08:05, Ali Juma <ajuma@chromium.org> wrote:
Is there a plan for how to handle web platform tests (where we can't add such a header)?
Currently, WebKitTestRunner enables all experimental features in tests (including those disabled by default), so adding an experimental feature (even one that's disabled by default) is a convenient way to get web platform test coverage for the feature. If WebKitTestRunner will no longer do that, we'll need some other way to keep these tests running.
I thought about this. I see three options:
1. Continue to enable all experimental features in testing. The downside is that we're testing something different to what we ship.
2. Have the default value only apply to testing, not to shipping. For Internal Features, instead of enabling them all, I had them revert to their default value.
3. Have some way to add the headers after importing the WPT test.
For now, I added headers to the WPT code, because there already were a bunch of tests that had such headers.
Is that automated? We don't want get into a state where we'd have to manually add those headers whenever we re-import tests. - R. Niwa
On 13 Sep 2018, at 08:50, Ryosuke Niwa <rniwa@webkit.org> wrote:
On Wed, Sep 12, 2018 at 3:09 PM Dean Jackson <dino@apple.com <mailto:dino@apple.com>> wrote:
On 13 Sep 2018, at 08:05, Ali Juma <ajuma@chromium.org <mailto:ajuma@chromium.org>> wrote:
Is there a plan for how to handle web platform tests (where we can't add such a header)?
Currently, WebKitTestRunner enables all experimental features in tests (including those disabled by default), so adding an experimental feature (even one that's disabled by default) is a convenient way to get web platform test coverage for the feature. If WebKitTestRunner will no longer do that, we'll need some other way to keep these tests running.
I thought about this. I see three options:
1. Continue to enable all experimental features in testing. The downside is that we're testing something different to what we ship.
2. Have the default value only apply to testing, not to shipping. For Internal Features, instead of enabling them all, I had them revert to their default value.
3. Have some way to add the headers after importing the WPT test.
For now, I added headers to the WPT code, because there already were a bunch of tests that had such headers.
Is that automated? We don't want get into a state where we'd have to manually add those headers whenever we re-import tests.
It isn't. I think maybe we should go for option 2 at the moment. Or, ultimately, have a per-directory configuration file that could enable features. Dean
On Wed, Sep 12, 2018 at 3:53 PM Dean Jackson <dino@apple.com> wrote:
On 13 Sep 2018, at 08:50, Ryosuke Niwa <rniwa@webkit.org> wrote:
On Wed, Sep 12, 2018 at 3:09 PM Dean Jackson <dino@apple.com> wrote:
On 13 Sep 2018, at 08:05, Ali Juma <ajuma@chromium.org> wrote:
Is there a plan for how to handle web platform tests (where we can't add such a header)?
Currently, WebKitTestRunner enables all experimental features in tests (including those disabled by default), so adding an experimental feature (even one that's disabled by default) is a convenient way to get web platform test coverage for the feature. If WebKitTestRunner will no longer do that, we'll need some other way to keep these tests running.
I thought about this. I see three options:
1. Continue to enable all experimental features in testing. The downside is that we're testing something different to what we ship.
2. Have the default value only apply to testing, not to shipping. For Internal Features, instead of enabling them all, I had them revert to their default value.
3. Have some way to add the headers after importing the WPT test.
For now, I added headers to the WPT code, because there already were a bunch of tests that had such headers.
Is that automated? We don't want get into a state where we'd have to manually add those headers whenever we re-import tests.
It isn't.
I think maybe we should go for option 2 at the moment.
Or, ultimately, have a per-directory configuration file that could enable features.
Okay. I can think of two options for (3): - Have a per-directory/per-test setting to enable/disable features - Have a per-directory/per-test importation rule to add headers. I think the second option is better because implicitly changing WTR/DRT's behavior based on directory names would be quite subtle for people who don't directly work on the test infrastructure. e.g. moving an imported test from one place to another would change the test's behavior. - R. Niwa
What about testing? ----
You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example...
<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] -->
It seems tedious to add such things to every test. That would also mean we would need to remove these lines to every test when we activate the flag by default or when we remove the flag. As of WPT, modifying tests on the fly at import time is doable but it goes against the idea of making WPT import/export simpler so I hope we can find something better. testharnessreport.js might help activating flags specifically for WPT. Since an experimental feature flag should not remain off for a long time, a per-experimental-feature file listing the tests to which activate the feature could be considered.
On 13 Sep 2018, at 09:03, youenn fablet <youennf@gmail.com> wrote:
What about testing? ----
You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example...
<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] -->
It seems tedious to add such things to every test. That would also mean we would need to remove these lines to every test when we activate the flag by default or when we remove the flag.
It's just a find and replace. I modified hundreds of tests in this manner (I didn't actually have to, since the headers were turning on something that was already on, but I did it anyway)
As of WPT, modifying tests on the fly at import time is doable but it goes against the idea of making WPT import/export simpler so I hope we can find something better.
testharnessreport.js might help activating flags specifically for WPT.
These features are not tweakable from the Web API, but from the client API. JS is too late.
Since an experimental feature flag should not remain off for a long time, a per-experimental-feature file listing the tests to which activate the feature could be considered.
That clashes with Ryosuke's point that moving tests shouldn't change behaviour. The larger problems are: - We don't want experimental features to be on in shipping browsers. It might be ok for them to be always on in tests, but then MiniBrowser would be different from WKTR. At the moment they are always on in tests, and use their default value in MiniBrowser (and shipping). - Internal features can either be on or off by default, because we don't expect users to toggle them. The tests should reflect their default value. You use the header to test the non-default case. So what we have right now is probably ok. The issue is with things I moved from experimental to internal and that have a default value of off. I think it is perfectly fine for those features to require test headers, since they clearly aren't ready. Note that even if I turned off all experimental features outside of testing, MiniBrowser and Safari remember if you enable the feature. Dean
Since an experimental feature flag should not remain off for a long time, a per-experimental-feature file listing the tests to which activate the feature could be considered.
That clashes with Ryosuke's point that moving tests shouldn't change behaviour.
I think it is better to discover the issue at the moment tests are moved. When moving/creating such tests in WPT, the potential issue will only arise after the export/import cycle. Chances are that the issue will get unnnoticed.
The larger problems are:
- We don't want experimental features to be on in shipping browsers. It might be ok for them to be always on in tests, but then MiniBrowser would be different from WKTR. At the moment they are always on in tests, and use their default value in MiniBrowser (and shipping).
When implementing a new feature, the first thing we usually do is having an experimental flag turned off by default. At some point, we turn on the experimental flag for tests. When ready, the flag is turned on by default. It is nice to keep the cost of implementing this workflow as low as possible. Having to tweak layout tests does not go in that direction. If this workflow was fully implementable by just updating WebPreferences.yaml, that would be great. That could also allow running mini browser in a mode where the same flags would be turned on as WKTR.
- Internal features can either be on or off by default, because we don't expect users to toggle them. The tests should reflect their default value. You use the header to test the non-default case.
So what we have right now is probably ok. The issue is with things I moved from experimental to internal and that have a default value of off. I think it is perfectly fine for those features to require test headers, since they clearly aren't ready.
If these features are for debug purposes only, it seems like they would never end up being turned on by default, in which case opt-in in tests is fine. I would think there would be no need to turn on these features for WPT tests.
On 13 Sep 2018, at 13:49, youenn fablet <youennf@gmail.com> wrote:
When implementing a new feature, the first thing we usually do is having an experimental flag turned off by default. At some point, we turn on the experimental flag for tests. When ready, the flag is turned on by default.
This isn't true. What we have is pretty strange. WKTR turns on all experimental features, no matter what their default is. This is a bit crazy because it means we're potentially developing three different configurations, and testing the one that none of us ever live on. 1. Dev build with experimental features as their default 2. Testing with experimental features all on 3. Shipping, where we hopefully remember to turn off any experimental features that default to on. Dean
Thanks for the heads-up. Some random thoughts on this: * For WPT tests, I would like a solution that does not discourage WebKit developers to contribute new tests to upstream WPT. It seems to me that such a risk exists for the proposal based on a header in the files. * The per-directory or per-file configuration allows to verify the new feature for a particular set of tests. However, enabling new feature often affects other existing tests and may require further adjustments. It is actually good to enable new features for tests as early as possible since it sometimes helps WebKit developers to detect issues they had not initially expected. Right now it happens when the feature is set to DEFAULT_EXPERIMENTAL_FEATURES_ENABLED. IIUC with the current proposal that will now only happen when the feature is removed from the experimental category and hence already considered stable enough for shipping. * Similarly, I wonder what will be the behavior of experimental features in Safari Tech Preview? I think currently they can be on by defaut and hence allows to get early feedback from Web developers. Again they might be affected by a new feature, even when they don't know about it at all (and hence would not try to explicitly enable it). IIUC, with the current proposal, they will always be off by default. * A bit tangential, but I wonder whether we should have a policy to handle web-facing changes. Mozilla and Chromium developers typically send "Intent to implement/ship/remove etc" to their developer mailing lists so that people are aware of the web-facing changes and can discuss them. As previously mentioned, people might only notice a behavior change once the new feature is enabled. IIUC, with the current proposal that will again only happen once the feature is no longer considered experimental. -- Frédéric Wang - frederic-wang.fr
On Wed, Sep 12, 2018 at 11:02 PM Frédéric Wang <fwang@igalia.com> wrote:
Thanks for the heads-up. Some random thoughts on this:
* For WPT tests, I would like a solution that does not discourage WebKit developers to contribute new tests to upstream WPT. It seems to me that such a risk exists for the proposal based on a header in the files.
Why? Let's say I've been writing tests in fast/shadow-dom and I want to export them to web-platform-tests/shadow-dom. The only thing I need to do is to update some configuration file which WPT importer uses to add those headers back when we import tests in that WPT directory. * The per-directory or per-file configuration allows to verify the new
feature for a particular set of tests. However, enabling new feature often affects other existing tests and may require further adjustments. It is actually good to enable new features for tests as early as possible since it sometimes helps WebKit developers to detect issues they had not initially expected. Right now it happens when the feature is set to DEFAULT_EXPERIMENTAL_FEATURES_ENABLED. IIUC with the current proposal that will now only happen when the feature is removed from the experimental category and hence already considered stable enough for shipping.
Yeah, I do share a similar concern. The idea that either the feature needs to be experimental or ready-to-ship doesn't seem right. There are moments in the time of developing a feature where we want to enable it for the purpose of testing yet shouldn't be turned on by default for most ports. But we don't need to impose a strict policy that we can't do this. It's probably fine for a feature to be transiently enabled by default only in WTR/DRT as the final validation step before the feature is turned on by default. * Similarly, I wonder what will be the behavior of experimental features
in Safari Tech Preview? I think currently they can be on by defaut and hence allows to get early feedback from Web developers. Again they might be affected by a new feature, even when they don't know about it at all (and hence would not try to explicitly enable it). IIUC, with the current proposal, they will always be off by default.
I think we don't want random features to be enabled by default in STP. We really want STP's feature state to be matching what we're intending to enable in Apple's macOS port. * A bit tangential, but I wonder whether we should have a policy to
handle web-facing changes. Mozilla and Chromium developers typically send "Intent to implement/ship/remove etc" to their developer mailing lists so that people are aware of the web-facing changes and can discuss them. As previously mentioned, people might only notice a behavior change once the new feature is enabled. IIUC, with the current proposal that will again only happen once the feature is no longer considered experimental.
I think contributors who are adding a new Web-facing feature or enabling an existing feature by default are expected to email webkit-dev first. I'd rather not make it a formal policy though. All these bureaucracies add up. - R. Niwa
On 12/09/2018 23:48, Dean Jackson wrote:
What about testing? ----
You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example...
<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] --> IIUC some platforms (WK1?) still use DumpRenderTree to run tests? Are such headers taken into account on these platforms too?
-- Frédéric Wang - frederic-wang.fr
On Mon, Sep 24, 2018 at 1:26 AM Frédéric Wang <fwang@igalia.com> wrote:
On 12/09/2018 23:48, Dean Jackson wrote:
What about testing? ----
You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example...
<!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] --> IIUC some platforms (WK1?) still use DumpRenderTree to run tests? Are such headers taken into account on these platforms too?
Both DRT & WTR parses webkit-test-runner flags just like "testRunner" object in DRT & WTR implements the same API. - R. Niwa
On 25/09/18 01:00, Ryosuke Niwa wrote:
On Mon, Sep 24, 2018 at 1:26 AM Frédéric Wang <fwang@igalia.com <mailto:fwang@igalia.com>> wrote:
On 12/09/2018 23:48, Dean Jackson wrote: > What about testing? > ---- > > You can turn both experimental and internal features on via headers in WebKitTestRunner. Use experimental:FeatureName or internal:FeatureName. For example... > > <!-- webkit-test-runner [ experimental:WebAnimationsCSSIntegrationEnabled=true ] --> IIUC some platforms (WK1?) still use DumpRenderTree to run tests? Are such headers taken into account on these platforms too?
Both DRT & WTR parses webkit-test-runner flags just like "testRunner" object in DRT & WTR implements the same API.
I'm not sure about DRT. It seems you need to modify https://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/TestOptions.cpp#L... in order that these headers work there. My 2 cents, Rego
On 05/11/2018 09:28, Manuel Rego Casasnovas wrote:
I'm not sure about DRT. It seems you need to modify https://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/TestOptions.cpp#L... in order that these headers work there. Thanks rego. Probably this file should assert or log error for unknown options, so that people realize they need to implement the parsing. I opened https://bugs.webkit.org/show_bug.cgi?id=191303
-- Frédéric Wang - frederic-wang.fr
participants (6)
-
Ali Juma
-
Dean Jackson
-
Frédéric Wang
-
Manuel Rego Casasnovas
-
Ryosuke Niwa
-
youenn fablet