Summary: Extend XHR Breakpoints to work for fetch requests as well. Rename UI to "Request Breakpoints" or something like that.
<rdar://problem/40431027>
Created attachment 358471 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Attachment 358471 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.h:92: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 1 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.
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"
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.
Created attachment 358514 [details] Patch
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.
Attachment 358514 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.h:92: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 358514 [details] Patch Clearing flags on attachment: 358514 Committed r239703: <https://trac.webkit.org/changeset/239703>
All reviewed patches have been landed. Closing bug.