(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.
Created attachment 378207 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 378208 [details] Patch oops, one minor fix
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.
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 :)
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
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?
(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.
(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.
(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).
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.
Created attachment 379111 [details] [Image] After Patch is applied (Settings Tab)
Created attachment 379112 [details] Patch Slightly adjusted explanation text content grammar and formatting.
Created attachment 379603 [details] Patch
Created attachment 379604 [details] [Image] After Patch is applied
Created attachment 379605 [details] Patch Rebase
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.
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
Created attachment 380811 [details] Patch
Comment on attachment 380811 [details] Patch Clearing flags on attachment: 380811 Committed r251039: <https://trac.webkit.org/changeset/251039>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56213014>