[webkit-reviews] review granted: [Bug 235216] Web Inspector: add a contextmenu item to create a URL Breakpoint for resources initiated by script : [Attachment 449127] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 235216: Web Inspector: add a contextmenu item to create a URL Breakpoint
for resources initiated by script
https://bugs.webkit.org/show_bug.cgi?id=235216

Attachment 449127: Patch

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




--- Comment #2 from Dean Jackson <dino at apple.com> ---
Comment on attachment 449127
  --> https://bugs.webkit.org/attachment.cgi?id=449127
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)