Bug 198855 – Web Inspector: Debugger: support pattern blackboxing

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

(In reply to Emanuele Feliziani from <https://webkit.org/b/17240#c8>)
> I'm jumping in to suggest folder-level blackboxing or even pattern-based, just as you have in Chrome. With today's JavaScript stacks, it's more convenient to just blackbox everything within node_modules.

[Devin Rousso]

Created attachment 378207 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 378208 [details]
Patch

oops, one minor fix

[Devin Rousso]

Created attachment 378209 [details]
[Image] After Patch is applied (Settings)

I'm open to suggestions for where we should put the blackbox pattern table, or if there's a different/better UI for it.

[Devin Rousso]

Created attachment 378210 [details]
[Image] After Patch is applied (WI.SourceCodeTreeElement)

I'm tempted to switch the UI so that regular URL blackboxing uses the "boxed" eye [<(*)>] and pattern blackboxing just has the greyed out eye <(*)>, as the "boxed" one looks more like a click target than the regular eye.  Many other apps just use an eye though, so that may be more familiar (precedent).

Any ideas of how to distinguish a between toggle-able URL blackbox (e.g. for a specific resource) vs a matched pattern blackbox (which can't be turned off for a specific resource, and instead matches an indeterminate number of URLs) are welcome :)

[EWS Watchlist]

Comment on attachment 378208 [details]
Patch

Attachment 378208 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13006492

New failing tests:
stress/test-out-of-memory.js.ftl-eager
stress/test-out-of-memory.js.dfg-eager-no-cjit-validate
stress/test-out-of-memory.js.ftl-eager-no-cjit

[Sam Weinig]

From the screenshots, it's not clear to me what this feature does. Can we make the UI in the settings more clear about what "blackboxing" is? Can we use a term that perhaps more folks are familiar with?

[Devin Rousso]

(In reply to Sam Weinig from comment #7)
> From the screenshots, it's not clear to me what this feature does. Can we make the UI in the settings more clear about what "blackboxing" is? Can we use a term that perhaps more folks are familiar with?
I'll add some explanatory text above the table that gives an idea of what blackboxing is, but I'm not sure there is a more applicable/accurate term (at least using just one word) than "blackboxing".  If you search for "blackboxing" on the internet, one of the first results that appears is the Chrome DevTools page about script blackboxing.  Once the Sources Tab becomes non-experimental, I'm also planning on writing a blog post about this feature so there will be more explanation available online as well.

[Sam Weinig]

(In reply to Devin Rousso from comment #8)
> (In reply to Sam Weinig from comment #7)
> > From the screenshots, it's not clear to me what this feature does. Can we make the UI in the settings more clear about what "blackboxing" is? Can we use a term that perhaps more folks are familiar with?
> I'll add some explanatory text above the table that gives an idea of what
> blackboxing is, but I'm not sure there is a more applicable/accurate term
> (at least using just one word) than "blackboxing".  If you search for
> "blackboxing" on the internet, one of the first results that appears is the
> Chrome DevTools page about script blackboxing.  Once the Sources Tab becomes
> non-experimental, I'm also planning on writing a blog post about this
> feature so there will be more explanation available online as well.

What's the text you plan on adding, maybe it will help me understand, provide feedback.

[Devin Rousso]

(In reply to Sam Weinig from comment #9)
> (In reply to Devin Rousso from comment #8)
> > (In reply to Sam Weinig from comment #7)
> > > From the screenshots, it's not clear to me what this feature does. Can we make the UI in the settings more clear about what "blackboxing" is? Can we use a term that perhaps more folks are familiar with?
> > I'll add some explanatory text above the table that gives an idea of what blackboxing is, but I'm not sure there is a more applicable/accurate term (at least using just one word) than "blackboxing".  If you search for "blackboxing" on the internet, one of the first results that appears is the Chrome DevTools page about script blackboxing.  Once the Sources Tab becomes non-experimental, I'm also planning on writing a blog post about this feature so there will be more explanation available online as well.
> 
> What's the text you plan on adding, maybe it will help me understand, provide feedback.
I think something along the lines of:
"""
    If the url of any script matches one of the regular expression patterns below, the debugger will defer any pauses that would've happened in that script until execution has continued outside of that script.
"""

That's the basic idea of what blackboxing is.

Our implementation has one nice feature in that we "carry forward" the pause reason and data (as much as we can) from the original pause in the script (which was deferred) to the new pause location outside of the script.

FWIW, individual script blackboxing (e.g. based on a specific load/instance of a specific resource, not just the URL) was added in r249450 <https://webkit.org/b/17240>.  This patch allows for URL based pattern blackboxing, which could be useful for situations where the URL slightly changes between sessions (e.g. a timestamp as a query parameter).

[Devin Rousso]

Created attachment 379110 [details]
Patch

Add explanatory text and fixed an issue where CodeMirror wouldn't show initial content due to being added to the DOM tree before the tree was attached to the main document.

[Devin Rousso]

Created attachment 379111 [details]
[Image] After Patch is applied (Settings Tab)

[Devin Rousso]

Created attachment 379112 [details]
Patch

Slightly adjusted explanation text content grammar and formatting.

[Devin Rousso]

Created attachment 379603 [details]
Patch

[Devin Rousso]

Created attachment 379604 [details]
[Image] After Patch is applied

[Devin Rousso]

Created attachment 379605 [details]
Patch

Rebase

[Timothy Hatcher]

Comment on attachment 379605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379605&action=review

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:168
> +            const shouldBlackbox = true;
> +            {
> +                const caseSensitive = true;

Nit: This style bothers me more than it likely should… but I'd add an empty line after const shouldBlackbox.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:173
> +            }
> +            {
> +                const isRegex = true;

Ditto. I would at least ad an empty line between these blocks (for my sanity.)

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:447
> +        console.assert(shouldBlackbox !== ((this.blackboxDataForSourceCode(sourceCode) || {}).type === DebuggerManager.BlackboxType.URL));

Nit: Group the assert with the asserts before and put the blank line below it instead?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:471
> +            let data = {
> +                url: regex.source,
> +                caseSensitive: !regex.ignoreCase,
> +            };

Nit: Blank lines before and after..

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:478
> +        }
> +        this._blackboxedPatternsSetting.save();

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Images/Hide.svg:27
> +        <symbol id="hide">

"hide" is an odd name, and could be confused. Maybe "eye"?

> Source/WebInspectorUI/UserInterface/Images/Hide.svg:36
> +    <svg id="currentColor"><use xlink:href="#hide"/></svg>
> +    <svg id="white"><use xlink:href="#hide"/></svg>
> +    <svg id="black"><use xlink:href="#hide"/></svg>

Clever use of vars, symbols and currentColor! Awesome that all works.

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:129
> +        });
> +        this._blackboxPatternCodeMirrorMap.set(regex, urlCodeMirror);

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:176
> +        };
> +        urlCodeMirror.addKeyMap({

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:180
> +        });
> +        urlCodeMirror.on("blur", update);

Nit: Blank line between.

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:183
> +        if (WI.DebuggerManager.supportsBlackboxingScripts()) {
> +            this._blackboxSettingsView = new WI.BlackboxSettingsView;
> +            this.addSettingsView(this._blackboxSettingsView);
> +        }

Nit: Blank lines before and after.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTreeElement.js:226
> +            }
> +            console.assert(this._toggleBlackboxedImageElement.title);

Nit: Blank line between.

[Devin Rousso]

Comment on attachment 379605 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=379605&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:471
>> +            };
> 
> Nit: Blank lines before and after..

I'd put one before, but I'd personally not add one after cause it's used right after and only right after, so it's "related".

>> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:129
>> +        this._blackboxPatternCodeMirrorMap.set(regex, urlCodeMirror);
> 
> Nit: Blank line between.

This is a pretty common pattern we have for adding objects we just created to some sort of collection.  I'd also "ditto" my last response.

>> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:176
>> +        urlCodeMirror.addKeyMap({
> 
> Nit: Blank line between.

Ditto

>> Source/WebInspectorUI/UserInterface/Views/BlackboxSettingsView.js:180
>> +        urlCodeMirror.on("blur", update);
> 
> Nit: Blank line between.

Ditto

[Devin Rousso]

Created attachment 380811 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 380811 [details]
Patch

Clearing flags on attachment: 380811

Committed r251039: <https://trac.webkit.org/changeset/251039>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ling Ho]

<rdar://problem/56213014>