Bug 203164 – Web Inspector: Provide a flag for technology preview builds

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Provide a flag for technology preview builds

In case there are features we want to enable for Technology Previews provide a single setting switch to enable them in non-TechnologyPreview builds.

[Joseph Pecoraro]

<rdar://problem/56202164>

[Joseph Pecoraro]

Created attachment 381327 [details]
[PATCH] Proposed Fix

[Matt Baker]

Comment on attachment 381327 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=381327&action=review

> Source/WebCore/inspector/InspectorFrontendHost.idl:86
> +    boolean isExperimentalBuild();

Why not `readonly attribute boolean isExperimentalBuild`, similar to `isRemote`?

[Joseph Pecoraro]

Comment on attachment 381327 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=381327&action=review

>> Source/WebCore/inspector/InspectorFrontendHost.idl:86
>> +    boolean isExperimentalBuild();
> 
> Why not `readonly attribute boolean isExperimentalBuild`, similar to `isRemote`?

They could both be booleans, I was just following the existing convention here. I don't think there is an advantage to either, so I'll keep these aligned.

[Joseph Pecoraro]

Created attachment 381337 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 381337 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=381337&action=review

r=me

> Source/WebInspectorUI/ChangeLog:16
> +        New flags.

NIT: I don't think this adds much.  I'd either add more description or leave it out.

> Source/WebInspectorUI/ChangeLog:20
> +        In non-TechnologyPreview builds, if there are Preview Features provide a

NIT: in some places you capitalize "Preview Features" and in others you keep them lowercase.  It'd be nice to be consistent one way or the other.

> Source/WebInspectorUI/UserInterface/Base/Setting.js:215
> +WI.previewFeatures = [];

Can we put the `[` and `]` on separate lines, so that if we do ever add anything to this the diff is smaller?

Are we expecting the values of this to be `WI.Setting`?  Perhaps we should call this `WI.previewFeatureSettings`?

> Source/WebInspectorUI/UserInterface/Base/Setting.js:220
> +}

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:352
> +        if ((WI.isTechnologyPreviewBuild() || WI.isEngineeringBuild) && hasPreviewFeatures) {

NIT: can we do `hasPreviewFeatures` first, so we don't even try to evaluate the others when we don't have any?  It would also line up nicely with the variable declaration on the previous line :)
```
    let hasPreviewFeatures = WI.previewFeatures.length > 0;
    if (hasPreviewFeatures && (WI.isTechnologyPreviewBuild() || WI.isEngineeringBuild)) {
```

[Joseph Pecoraro]

Comment on attachment 381337 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=381337&action=review

>> Source/WebInspectorUI/UserInterface/Base/Setting.js:215
>> +WI.previewFeatures = [];
> 
> Can we put the `[` and `]` on separate lines, so that if we do ever add anything to this the diff is smaller?
> 
> Are we expecting the values of this to be `WI.Setting`?  Perhaps we should call this `WI.previewFeatureSettings`?

I guess we can cross that bridge when we have a preview feature. Heck it could just be a string. I did imagine it being a setting in case we are graduating a setting (that way it would be easy to search for the use of the settings and find it in this list). But for a feature that doesn't even have a setting it could just be a string that we remove soon after. I don't know how frequently, if ever, we are going to need to use this list.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:352
>> +        if ((WI.isTechnologyPreviewBuild() || WI.isEngineeringBuild) && hasPreviewFeatures) {
> 
> NIT: can we do `hasPreviewFeatures` first, so we don't even try to evaluate the others when we don't have any?  It would also line up nicely with the variable declaration on the previous line :)
> ```
>     let hasPreviewFeatures = WI.previewFeatures.length > 0;
>     if (hasPreviewFeatures && (WI.isTechnologyPreviewBuild() || WI.isEngineeringBuild)) {
> ```

Nice!

[Joseph Pecoraro]

<https://trac.webkit.org/r251379>