Summary: | Web Inspector: Settings: add an Engineering pane to expose useful settings for other WebKit engineers | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, joepeck, nvasilyev, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Devin Rousso
2019-08-06 17:10:01 PDT
Created attachment 375738 [details]
Patch
Created attachment 375740 [details]
[Image] After Patch is applied (Engineering pane of Settings Tab)
Created attachment 375742 [details]
[Image] After Patch is applied (Debug pane of Settings Tab)
Could you re-title the bug mentioning the UI changes, e.g. "Add Engineering panel to Settings"? Now it sounds like a refactoring. Created attachment 375750 [details]
Patch
Comment on attachment 375750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375750&action=review rs=me > Source/WebInspectorUI/UserInterface/Base/Main.js:2707 > + if (!WI.isDebugUIEnabled() || layoutDirection === WI.LayoutDirection.System) No particular reason to include these checks. This just just makes this localStorage value only take effect if another localStorage value is set. Seems weird. > Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:154 > + if (target.DebuggerAgent.setPauseForInternalScripts) { > + if (WI.isEngineeringBuild && WI.settings.engineeringPauseForInternalScripts.value) > + target.DebuggerAgent.setPauseForInternalScripts(WI.settings.engineeringPauseForInternalScripts.value); > + } We could move the isEngineeringBuild check at the top level: if (WI.isEngineeringBuild) { if (target.DebuggerAgent.setPauseForInternalScripts) target.DebuggerAgent.setPauseForInternalScripts(WI.settings.engineeringPauseForInternalScripts.value); } > Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:36 > +WI.settings.engineeringShowInternalScripts.addEventListener(WI.Setting.Event.Changed, (event) => { > + if (!WI.settings.engineeringShowInternalScripts.value) > + WI.settings.engineeringPauseForInternalScripts.value = false; > +}, WI.settings.engineeringPauseForInternalScripts); > + > +WI.settings.engineeringPauseForInternalScripts.addEventListener(WI.Setting.Event.Changed, (event) => { > + if (WI.settings.engineeringPauseForInternalScripts.value) > + WI.settings.engineeringShowInternalScripts.value = true; > +}, WI.settings.engineeringShowInternalScripts); The `thisObject` is not needed for either of these. I think these could use a comment. // Disable Pause in Internal Scripts if Show Internal Scripts is dibbled. // Enable Show Internal Scripts if Pause in Internal Scripts is enabled. Comment on attachment 375750 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375750&action=review >> Source/WebInspectorUI/UserInterface/Base/Main.js:2707 >> + if (!WI.isDebugUIEnabled() || layoutDirection === WI.LayoutDirection.System) > > No particular reason to include these checks. This just just makes this localStorage value only take effect if another localStorage value is set. Seems weird. It's actually a twofold check, as it early returns if `WI.showDebugUISetting` isn't set (which is the case for non-engineering builds). It would only check the `localStorage` value if we're in an engineering build. >> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:36 >> +}, WI.settings.engineeringShowInternalScripts); > > The `thisObject` is not needed for either of these. > > I think these could use a comment. > > // Disable Pause in Internal Scripts if Show Internal Scripts is dibbled. > // Enable Show Internal Scripts if Pause in Internal Scripts is enabled. I realize that `thisObject` isn't needed, but if we move event listeners to using a `WeakMap`/`WeakRef`, we would likely always need to provide a `thisObject` so that the `callback` isn't collected. I'll add the comments :) Created attachment 375754 [details]
Patch
Comment on attachment 375754 [details] Patch Clearing flags on attachment: 375754 Committed r248391: <https://trac.webkit.org/changeset/248391> All reviewed patches have been landed. Closing bug. |