[webkit-reviews] review granted: [Bug 200492] Web Inspector: Settings: add an Engineering pane to expose useful settings for other WebKit engineers : [Attachment 375750] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 200492: Web Inspector: Settings: add an Engineering pane to expose useful
settings for other WebKit engineers
https://bugs.webkit.org/show_bug.cgi?id=200492

Attachment 375750: Patch

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




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 375750
  --> https://bugs.webkit.org/attachment.cgi?id=375750
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.engineeringPauseFor
InternalScripts.value);
> +	   }

We could move the isEngineeringBuild check at the top level:

    if (WI.isEngineeringBuild) {
	if (target.DebuggerAgent.setPauseForInternalScripts)
	   
target.DebuggerAgent.setPauseForInternalScripts(WI.settings.engineeringPauseFor
InternalScripts.value);
    }

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:36
>
+WI.settings.engineeringShowInternalScripts.addEventListener(WI.Setting.Event.C
hanged, (event) => {
> +    if (!WI.settings.engineeringShowInternalScripts.value)
> +	   WI.settings.engineeringPauseForInternalScripts.value = false;
> +}, WI.settings.engineeringPauseForInternalScripts);
> +
>
+WI.settings.engineeringPauseForInternalScripts.addEventListener(WI.Setting.Eve
nt.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.