[webkit-reviews] review granted: [Bug 215793] Web Inspector: allow url breakpoints to be configured : [Attachment 407544] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 215793: Web Inspector: allow url breakpoints to be configured
https://bugs.webkit.org/show_bug.cgi?id=215793

Attachment 407544: Patch

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




--- Comment #3 from Brian Burg <bburg at apple.com> ---
Comment on attachment 407544
  --> https://bugs.webkit.org/attachment.cgi?id=407544
Patch

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

r=me, great cleanup!

Please rebase and run through EWS prior to landing.

> LayoutTests/inspector/debugger/resources/breakpoint-options-utilities.js:56
> +		   await resumePromise;

Oops.

>
LayoutTests/inspector/dom-debugger/url-breakpoints-all-requests-expected.txt:14
> +Adding "text" URL Breakpoint...

Uh, why "text"?

>
LayoutTests/inspector/dom-debugger/url-breakpoints-all-requests-expected.txt:14
3
> +-- Running test case: URLBreakpoint.BreakOnAllXHR..Options.Condition

Nit: Extra ..

>
LayoutTests/inspector/dom-debugger/url-breakpoints-all-requests-expected.txt:18
7
> +-- Running test case: URLBreakpoint.BreakOnAllXHR..Options.Action.Log

Ditto

>
LayoutTests/inspector/dom-debugger/url-breakpoints-all-requests-expected.txt:21
9
> +-- Running test case: URLBreakpoint.BreakOnAllXHR..Options.Actions.Evaluate

Ditto.

> Source/JavaScriptCore/ChangeLog:9
> +	   Add an `options` paramater to `DOMDebugger.setURLBreakpoint` to
allow configuration.

Nit: parameter

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:725
> +	   if (target.hasCommand("DOMDebugger.setXHRBreakpoint")) {

Nit: comment and the command name don't match. Please add separate
COMPATIBILITY for setXHRBreakpoint and setURLBreakpoint.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:734
> +	   if (!target.hasCommand("DOMDebugger.setURLBreakpoint"))

.. down here.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:750
> +	   if (target.hasCommand("DOMDebugger.removeXHRBreakpoint")) {

Ditto to the above COMPATIBILITY note.