WI.arePreviewFeaturesEnabled() correctly returns true by default on Engineering and Preview builds. However, this setting Staging: [ ] Enable Preview Features doesn't affect WI.arePreviewFeaturesEnabled(), which is used to determine whether or not we show the new P3 color picker. If the setting is unchecked, WI.arePreviewFeaturesEnabled() still returns true.
<rdar://problem/57037089>
Created attachment 383172 [details] Patch
Comment on attachment 383172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383172&action=review I don't understand the need for a separate WI.canShowPreviewFeatures(). > Source/WebInspectorUI/UserInterface/Base/Setting.js:189 > + experimentalEnablePreviewFeatures: new WI.Setting("experimental-enable-preview-features", true), I'm not sure what to think here. Why has the default changed? I would assume Safari Technology Preview should always show experimental features, whereas production builds should hide them behind a checkbox? In that case the default should depend on if (WI.isExperimentalBuild && !WI.isEngineeringBuild) > Source/WebInspectorUI/UserInterface/Base/Setting.js:223 > + return hasPreviewFeatures && (WI.isExperimentalBuild || WI.isEngineeringBuild); This is redundant. Engineering builds always define ENABLE(EXPERIMENTAL_FEATURES). > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:349 > + if (WI.canShowPreviewFeatures()) { Please just make this if (WI.arePreviewFeaturesEnabled() && WI.previewFeatures.length)
Comment on attachment 383172 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383172&action=review All I care is to be able disable & enable preview preview features in the engineering build. Now I can't do that because the checkbox does nothing. >> Source/WebInspectorUI/UserInterface/Base/Setting.js:189 >> + experimentalEnablePreviewFeatures: new WI.Setting("experimental-enable-preview-features", true), > > I'm not sure what to think here. Why has the default changed? > > I would assume Safari Technology Preview should always show experimental features, whereas production builds should hide them behind a checkbox? In that case the default should depend on > > if (WI.isExperimentalBuild && !WI.isEngineeringBuild) Production builds hide preview features and don't even show the checkbox. This patch didn't change that. This patch allows to disable preview features in engineering and STP builds. Before this patch the setting existed but the checkbox wouldn't actually do anything. Re: "Why has the default changed?". Preview features should be enabled by default in engineering and STP builds. Changing the default was the most straightforward fix. Let me know if you have other suggestions. >> Source/WebInspectorUI/UserInterface/Base/Setting.js:223 >> + return hasPreviewFeatures && (WI.isExperimentalBuild || WI.isEngineeringBuild); > > This is redundant. Engineering builds always define ENABLE(EXPERIMENTAL_FEATURES). Thanks. >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:349 >> + if (WI.canShowPreviewFeatures()) { > > Please just make this > > if (WI.arePreviewFeaturesEnabled() && WI.previewFeatures.length) This is used in two different places and it has a comment. I thought I would make sense not to repeat it.
Comment on attachment 383172 [details] Patch Please rebase the patch and address the redundant check I pointed out, thanks.
Created attachment 388086 [details] Patch Rebaselined.
Comment on attachment 388086 [details] Patch r=me
The commit-queue encountered the following flaky tests while processing attachment 388086 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 388086 [details] Patch Clearing flags on attachment: 388086 Committed r254773: <https://trac.webkit.org/changeset/254773>
All reviewed patches have been landed. Closing bug.