Bug 185843 - Web Inspector: extend XHR breakpoints to work with fetch
Summary: Web Inspector: extend XHR breakpoints to work with fetch
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:
Blocks:
 
Reported: 2018-05-21 14:59 PDT by Matt Baker
Modified: 2019-01-07 14:41 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 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.
Comment 1 Radar WebKit Bug Importer 2018-05-21 15:00:12 PDT
<rdar://problem/40431027>
Comment 2 Devin Rousso 2019-01-06 17:56:33 PST
Created attachment 358471 [details]
Patch
Comment 3 EWS Watchlist 2019-01-06 17:59:21 PST Comment hidden (obsolete)
Comment 4 EWS Watchlist 2019-01-06 17:59:32 PST Comment hidden (obsolete)
Comment 5 Matt Baker 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"
Comment 6 Joseph Pecoraro 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.
Comment 7 Devin Rousso 2019-01-07 12:39:11 PST
Created attachment 358514 [details]
Patch
Comment 8 Devin Rousso 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.
Comment 9 EWS Watchlist 2019-01-07 12:41:53 PST Comment hidden (obsolete)
Comment 10 WebKit Commit Bot 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>
Comment 11 WebKit Commit Bot 2019-01-07 14:41:41 PST
All reviewed patches have been landed.  Closing bug.