Summary: | Web Inspector: Provide a flag for technology preview builds | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2019-10-18 14:16:24 PDT
Created attachment 381327 [details]
[PATCH] Proposed Fix
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`? 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. Created attachment 381337 [details]
[PATCH] Proposed Fix
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)) { ``` 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! |