Other WebKit engineers might find being able to see internal objects or pause in internal scripts useful, so we should allow them to do so without having to enable Web Inspector's debug "mode".
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.
<rdar://problem/54051842>