Bug 185843 – Web Inspector: extend XHR breakpoints to work with fetch

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Extend XHR Breakpoints to work for fetch requests as well. Rename UI to "Request Breakpoints" or something like that.

[Radar WebKit Bug Importer]

<rdar://problem/40431027>

[Devin Rousso]

Created attachment 358471 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS Watchlist]

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 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 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]

Created attachment 358514 [details]
Patch

[Devin Rousso]

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]

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 on attachment 358514 [details]
Patch

Clearing flags on attachment: 358514

Committed r239703: <https://trac.webkit.org/changeset/239703>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.