Bug 204026 – Web Inspector: Unchecking Enable Preview Features on Engineering and Preview builds does not affect WI.arePreviewFeaturesEnabled()

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/57037089>

[Nikita Vasilyev]

Created attachment 383172 [details]
Patch

[Blaze Burg]

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)

[Nikita Vasilyev]

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.

[Blaze Burg]

Comment on attachment 383172 [details]
Patch

Please rebase the patch and address the redundant check I pointed out, thanks.

[Nikita Vasilyev]

Created attachment 388086 [details]
Patch

Rebaselined.

[Blaze Burg]

Comment on attachment 388086 [details]
Patch

r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 388086 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 388086 [details]
Patch

Clearing flags on attachment: 388086

Committed r254773: <https://trac.webkit.org/changeset/254773>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.