Bug 200492 – Web Inspector: Settings: add an Engineering pane to expose useful settings for other WebKit engineers

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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".

[Devin Rousso]

Created attachment 375738 [details]
Patch

[Devin Rousso]

Created attachment 375740 [details]
[Image] After Patch is applied (Engineering pane of Settings Tab)

[Devin Rousso]

Created attachment 375742 [details]
[Image] After Patch is applied (Debug pane of Settings Tab)

[Nikita Vasilyev]

Could you re-title the bug mentioning the UI changes, e.g. "Add Engineering panel to Settings"? Now it sounds like a refactoring.

[Devin Rousso]

Created attachment 375750 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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 :)

[Devin Rousso]

Created attachment 375754 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375754 [details]
Patch

Clearing flags on attachment: 375754

Committed r248391: <https://trac.webkit.org/changeset/248391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54051842>