this would also help expose the fact that URL Breakpoints exist :)
Created attachment 449127 [details] Patch
Comment on attachment 449127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449127&action=review > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:470 > + .filter((urlBreakpoint) => { Is it normal style to wrap a single parameter in ()? > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:487 > + const typeRankings = [ > + WI.URLBreakpoint.Type.Text, > + WI.URLBreakpoint.Type.RegularExpression, > + ]; > + return typeRankings.indexOf(a.type) - typeRankings.indexOf(b.type); This is cute. > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:138 > + contextMenu.appendItem(existingURLBreakpoints.length === 1 ? WI.UIString("Delete URL Breakpoint") : WI.UIString("Delete URL Breakpoints"), () => { Do you really need === here, when checking against a literal number? > Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:140 > + for (let urlBreakpoint of existingURLBreakpoints) > + WI.domDebuggerManager.removeURLBreakpoint(urlBreakpoint); Why not this shorter code? existingURLBreakpoints.forEach(WI.domDebuggerManager.removeURLBreakpoint)
Comment on attachment 449127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449127&action=review Thanks for the review! =D >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:470 >> + .filter((urlBreakpoint) => { > > Is it normal style to wrap a single parameter in ()? Technically it's not necessary, but it is Web Inspector style to always add the `(` `)` <https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#ArrowFunctions> >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:487 >> + return typeRankings.indexOf(a.type) - typeRankings.indexOf(b.type); > > This is cute. :) >> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:138 >> + contextMenu.appendItem(existingURLBreakpoints.length === 1 ? WI.UIString("Delete URL Breakpoint") : WI.UIString("Delete URL Breakpoints"), () => { > > Do you really need === here, when checking against a literal number? Do you mean using `==` instead? It's generally a bad idea to use `==` (even with a constant) in JS cause it doesn't also check the type (e.g. `"1" == 1` is true whereas `"1" === 1` is not). The ESLint config for Web Inspector will warn you if you try to use `==` instead of `===` for that reason. >> Source/WebInspectorUI/UserInterface/Views/ContextMenuUtilities.js:140 >> + WI.domDebuggerManager.removeURLBreakpoint(urlBreakpoint); > > Why not this shorter code? > > existingURLBreakpoints.forEach(WI.domDebuggerManager.removeURLBreakpoint) We typically only use `forEach` when doing functional-style chaining. Also, it makes it harder to find callsites of functions (and could possibly cause unintended side effects if the expected parameters of the passed in callback don't match `(item, index, array)`). <https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#APIpreferences>
Committed r288029 (246055@main): <https://commits.webkit.org/246055@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449127 [details].
<rdar://problem/87616390>