[webkit-reviews] review denied: [Bug 170033] Web Inspector: XHR breakpoints should be global : [Attachment 305297] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Matt Baker
<mattbaker at apple.com>'s request for review:
Bug 170033: Web Inspector: XHR breakpoints should be global
https://bugs.webkit.org/show_bug.cgi?id=170033

Attachment 305297: Patch

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




--- Comment #4 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 305297
  --> https://bugs.webkit.org/attachment.cgi?id=305297
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:186
> +	   if (this._xhrBreakpoints.some((entry) => entry.url ===
breakpoint.url))
> +	       return;

How does the UI handle this? What does the UI display when the user adds the
same URL breakpoint twice? It looks like the UI would show it twice, until we
navigate and then it would show 1. That might need to be improved.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:363
> +	      
breakpoint.dispatchEventToListeners(WebInspector.XHRBreakpoint.Event.XHRBreakpo
intResolved);

My checkout doesn't have this event name. What is this? r- for this

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:112
>  .sidebar > .panel.navigation.debugger .details-section.xhr-breakpoints
.item.breakpoint .subtitle {
> -    padding-left: 5px;
>      font-family: Menlo, monospace;

Food for thought: If we supported regular expressions (which I think we should
because it would be a very small patch) we could use a FormattedValue here. We
could show:

    URL – "foo"
    URL – /foo\d/

or:

    URL contains: "foo"
    URL matches: /foo\d/

And they would be syntax highlighted like they are in the console. What you
have now looks good to me, far more compact.