RESOLVED FIXED 185843
Web Inspector: extend XHR breakpoints to work with fetch
https://bugs.webkit.org/show_bug.cgi?id=185843
Summary Web Inspector: extend XHR breakpoints to work with fetch
Matt Baker
Reported 2018-05-21 14:59:49 PDT
Summary: Extend XHR Breakpoints to work for fetch requests as well. Rename UI to "Request Breakpoints" or something like that.
Attachments
Patch (103.82 KB, patch)
2019-01-06 17:56 PST, Devin Rousso
no flags
Patch (114.94 KB, patch)
2019-01-07 12:39 PST, Devin Rousso
no flags
Radar WebKit Bug Importer
Comment 1 2018-05-21 15:00:12 PDT
Devin Rousso
Comment 2 2019-01-06 17:56:33 PST
EWS Watchlist
Comment 3 2019-01-06 17:59:21 PST Comment hidden (obsolete)
EWS Watchlist
Comment 4 2019-01-06 17:59:32 PST Comment hidden (obsolete)
Matt Baker
Comment 5 2019-01-07 11:24:07 PST
Comment on attachment 358471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358471&action=review r=me, nice refactoring. I think we should combine fetch-breakpoints.html and xhr-breakpoints.html, since the files are nearly identical. > Source/WebInspectorUI/UserInterface/Models/URLBreakpoint.js:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. 2019 > Source/WebInspectorUI/UserInterface/Views/URLBreakpointPopover.css:2 > + * Copyright (C) 2017 Apple Inc. All rights reserved. Ditto for all these copy-pasted files. > LayoutTests/inspector/dom-debugger/fetch-breakpoints-expected.txt:19 > +Sending Fetch. Nit: logging the URL improves the clarity of tests that that URL matches a string/regular expression. Sending Fetch -> Fetch "dataFetch.json"
Joseph Pecoraro
Comment 6 2019-01-07 11:39:28 PST
Comment on attachment 358471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358471&action=review > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:-123 > - m_xhrBreakpoints.clear(); Should this now include `m_urlBreakpoints.clear()`? > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:468 > + Inspector::DebuggerFrontendDispatcher::Reason breakReason; > + switch (source) { > + case URLBreakpointSource::Fetch: > + breakReason = Inspector::DebuggerFrontendDispatcher::Reason::Fetch; > + break; > + > + case URLBreakpointSource::XHR: > + breakReason = Inspector::DebuggerFrontendDispatcher::Reason::XHR; > + break; > + > + default: > + ASSERT_NOT_REACHED(); > + breakReason = Inspector::DebuggerFrontendDispatcher::Reason::Other; > + break; > + } Style: I like putting these conversions into a static helper function. You can then use `return` instead of assign + break statements so it ends up more concise. > Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.h:114 > + bool m_pauseOnAllURLsEnabled { false }; This value should be getting reset somewhere, probably in `disable()`, back to `false` when a frontend disconnects. > Source/WebInspectorUI/UserInterface/Base/Setting.js:48 > + static takeDeprecatedValue(key) How about naming this something like: `migrateValue(key)`? > Source/WebInspectorUI/UserInterface/Base/Setting.js:56 > + delete window.localStorage[key]; It might be cleaner to do: window.localStorage.removeItem(key); That should avoid any possible weirdness with the `delete` operator and `[key]` indexing. Though I realize we use `delete ...` in other places so maybe just keep this to be consistent. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:518 > + > + Style: Extra newline.
Devin Rousso
Comment 7 2019-01-07 12:39:11 PST
Devin Rousso
Comment 8 2019-01-07 12:39:45 PST
Comment on attachment 358471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358471&action=review >> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:-123 >> - m_xhrBreakpoints.clear(); > > Should this now include `m_urlBreakpoints.clear()`? I think this should just be moved to `InspectorDOMAgent::disable`. It doesn't make sense to clear url/event breakpoints when the main frame navigates, as there's nothing page-specific about either.
EWS Watchlist
Comment 9 2019-01-07 12:41:53 PST Comment hidden (obsolete)
WebKit Commit Bot
Comment 10 2019-01-07 14:41:39 PST
Comment on attachment 358514 [details] Patch Clearing flags on attachment: 358514 Committed r239703: <https://trac.webkit.org/changeset/239703>
WebKit Commit Bot
Comment 11 2019-01-07 14:41:41 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.