Summary: | Web Inspector: Unchecking Enable Preview Features on Engineering and Preview builds does not affect WI.arePreviewFeaturesEnabled() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, commit-queue, inspector-bugzilla-changes, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-11-08 15:19:30 PST
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. |