Bug 215793 - Web Inspector: allow url breakpoints to be configured
Summary: Web Inspector: allow url breakpoints to be configured
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 215362 215363 215747 215794
Blocks:
  Show dependency treegraph
 
Reported: 2020-08-24 21:40 PDT by Devin Rousso
Modified: 2020-09-03 13:07 PDT (History)
11 users (show)

See Also:


Attachments
Patch (112.24 KB, patch)
2020-08-29 12:40 PDT, Devin Rousso
bburg: review+
hi: commit-queue-
Details | Formatted Diff | Diff
Patch (113.05 KB, patch)
2020-09-02 18:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2020-08-24 21:40:59 PDT
This would allow developers to do things like:
 - ignore the first N requests
 - evaluate JavaScript whenever a network request is dispatched without pausing
Comment 1 Devin Rousso 2020-08-29 12:40:58 PDT
Created attachment 407544 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2020-08-31 21:41:12 PDT
<rdar://problem/68118986>
Comment 3 BJ Burg 2020-09-01 16:08:35 PDT
Comment on attachment 407544 [details]
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:143
> +-- Running test case: URLBreakpoint.BreakOnAllXHR..Options.Condition

Nit: Extra ..

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

Ditto

> LayoutTests/inspector/dom-debugger/url-breakpoints-all-requests-expected.txt:219
> +-- 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.
Comment 4 Devin Rousso 2020-09-01 16:51:45 PDT
Comment on attachment 407544 [details]
Patch

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

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

The All Requests breakpoint is a text URL breakpoint (i.e. not regular expression) that has no URL.  I'll special case that in the logging :)

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

The compatibility comment here is that `setXHRBreakpoint` is what existed before `setURLBreakpoint` replaced it (when `fetch` support was added).

I agree that this is confusing tho, so I'll add separate comments for each :)
Comment 5 Devin Rousso 2020-09-02 18:56:27 PDT
Created attachment 407849 [details]
Patch

Also ensure that modifying the options of a `WI.EventBreakpoint` doesn't enable breakpoints globally.  This matches what is done with `WI.JavaScriptBreakpoint`.
Comment 6 EWS Watchlist 2020-09-03 10:56:10 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 7 EWS 2020-09-03 13:07:44 PDT
Committed r266538: <https://trac.webkit.org/changeset/266538>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407849 [details].