[webkit-reviews] review granted: [Bug 203164] Web Inspector: Provide a flag for technology preview builds : [Attachment 381337] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 203164: Web Inspector: Provide a flag for technology preview builds
https://bugs.webkit.org/show_bug.cgi?id=203164

Attachment 381337: [PATCH] Proposed Fix

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




--- Comment #6 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 381337
  --> https://bugs.webkit.org/attachment.cgi?id=381337
[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)) {
```