Summary: | Web Inspector: allow url breakpoints to be configured | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Devin Rousso <hi> | ||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 215362, 215363, 215747, 215794 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Devin Rousso
2020-08-24 21:40:59 PDT
Created attachment 407544 [details]
Patch
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 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 :) 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`.
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. Committed r266538: <https://trac.webkit.org/changeset/266538> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407849 [details]. |