Bug 235216 – Web Inspector: add a contextmenu item to create a URL Breakpoint for resources initiated by script

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

this would also help expose the fact that URL Breakpoints exist :)

[Devin Rousso]

Created attachment 449127 [details]
Patch

[Dean Jackson]

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)

[Devin Rousso]

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>

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/87616390>