WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(114.94 KB, patch)
2019-01-07 12:39 PST
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-05-21 15:00:12 PDT
<
rdar://problem/40431027
>
Devin Rousso
Comment 2
2019-01-06 17:56:33 PST
Created
attachment 358471
[details]
Patch
EWS Watchlist
Comment 3
2019-01-06 17:59:21 PST
Comment hidden (obsolete)
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
EWS Watchlist
Comment 4
2019-01-06 17:59:32 PST
Comment hidden (obsolete)
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.
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
Created
attachment 358514
[details]
Patch
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)
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.
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.
Top of Page
Format For Printing
XML
Clone This Bug