Bug 168763 – Web Inspector: Add XHR breakpoints UI

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

InspectorDOMDebuggerAgent has support for XHR breakpoints already; this issue covers UI and layout tests.

This builds out the DOMDebugger domain frontend classes added in https://bugs.webkit.org/show_bug.cgi?id=168101.

[Matt Baker]

Created attachment 303938 [details]
[Image] New UI

[Radar WebKit Bug Importer]

<rdar://problem/30952439>

[Radar WebKit Bug Importer]

<rdar://problem/30952433>

[Matt Baker]

Created attachment 303954 [details]
Patch

[Matt Baker]

Created attachment 303955 [details]
Patch

[Matt Baker]

Comment on attachment 303955 [details]
Patch

cq- for now. Fixing XHR breakpoint tree element UI.

[Matt Baker]

Created attachment 303964 [details]
[Image] Final UI

Polished the breakpoint title/subtitle

[Matt Baker]

Created attachment 303967 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 303967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303967&action=review

(Review not done yet)

Kinda difficult to review this when the backend is completely invisible in this patch. These comments apply to those files:

  - InspectorDOMDebuggerAgent needs to clear m_xhrBreakpoints in disable().
  - Other methods return if breakpoints are not enabled, seems willSendXHR will need to.
  - Protocol methods are named setXHRBreakpoint / removeXHRBreakpoint. If we want this to apply to Fetch later it seems we would want to rename this.
  - Protocol methods should mention that if URL is empty it applies to ALL XHRs. That is entirely non-obvious.

I know we want to support Fetch. I hope we can move to better naming to treat XHR / Fetch the same without having duplicate sections. Just about everywhere this patch includes "XHR" would probably want to change if we were to make this generic to XHR + Fetch. For example, Settings, Class Names, File Names, Protocol Names, etc.

XHRs and Fetch both work in Workers, so these should work with Workers just like regular breakpoints.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:111
> +        let breakpoints = this._xhrBreakpointURLMap.get(mainFrame.url)
> +        if (!breakpoints)
> +            return [];

I'm not sure I understand why we would limit XHR breakpoints based on the main page's URL. This feels wrong. What about navigations? What if the page is put inside of an iframe? Regular breakpoints handle these cases.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:194
> +        let url = breakpoint.documentURL;
> +        let breakpoints = this._xhrBreakpointURLMap.get(url);

Yeah, we should revisit this, I don't like the idea of storing breakpoints per-document URL. If I have a framework used by all my pages, I'd want it to apply no matter what page I'm on.

> Source/WebInspectorUI/UserInterface/Views/InputPopover.js:96
> +    Committed: Symbol("result-cancelled"),

Fix the name.

[Matt Baker]

Comment on attachment 303967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303967&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:111
>> +            return [];
> 
> I'm not sure I understand why we would limit XHR breakpoints based on the main page's URL. This feels wrong. What about navigations? What if the page is put inside of an iframe? Regular breakpoints handle these cases.

Since the behavior is dictated by the frontend, I will investigate and address in a follow-up.

[Matt Baker]

(In reply to comment #9)
> Comment on attachment 303967 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303967&action=review
> 
> (Review not done yet)
> 
> Kinda difficult to review this when the backend is completely invisible in
> this patch. These comments apply to those files:
> 
>   - InspectorDOMDebuggerAgent needs to clear m_xhrBreakpoints in disable().
>   - Other methods return if breakpoints are not enabled, seems willSendXHR
> will need to.
Fixed.

>   - Protocol methods are named setXHRBreakpoint / removeXHRBreakpoint. If we
> want this to apply to Fetch later it seems we would want to rename this.
I would rename the protocol methods now, but the signature may change if we add a parameter to control whether the breakpoint applies to XHR/fetch/or both.

>   - Protocol methods should mention that if URL is empty it applies to ALL
> XHRs. That is entirely non-obvious.
Fixed.

[Joseph Pecoraro]

Comment on attachment 303967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303967&action=review

r=me

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:347
> +        DOMDebuggerAgent.removeXHRBreakpoint(breakpoint.url, (error) => {

Please file a follow-up to get this working with Workers. XHR / Fetch work in Workers, so we will want these breakpoints to work there as well.

> Source/WebInspectorUI/UserInterface/Models/XHRBreakpoint.js:32
> +        this._documentURL = documentURL;

Yeah, I really don't understand this constraint.

> Source/WebInspectorUI/UserInterface/Views/ContentView.js:130
> +        if (representedObject instanceof WebInspector.XHRBreakpoint)
> +            return WebInspector.ContentView.createFromRepresentedObject(WebInspector.frameResourceManager.mainFrame.domTree, extraArguments);

So when you click on an XHR breakpoint it takes you to the DOM?! I don't think any view makes sense. Image if we have a breakpoint later on for "setTimeout" that wouldn't be able to take you anywhere.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:193
> +            let addXHRBreakpointButton = new WebInspector.ButtonNavigationItem("add-xhr-breakpoint", WebInspector.UIString("Add XHR breakpoint"), "Images/Plus13.svg", 13, 13);

Everything else uses capital 'B' Breakpoints. This one probably should too.

This looks a bit large in the screenshot but we can improve the appearance later.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:369
> +        else if (cookie[WebInspector.DebuggerSidebarPanel.SelectedAllRequestsCookieKey]) {

Style: No braces.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1128
> +        let popover = new WebInspector.InputPopover(WebInspector.UIString("Break when URL contains:"), this);

A screenshot of this popover would have been useful.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1145
> +        let documentURL = WebInspector.frameResourceManager.mainFrame.url;
> +        WebInspector.domDebuggerManager.addXHRBreakpoint(new WebInspector.XHRBreakpoint(documentURL, url));

The backend uses a HashSet of URLs. That means if duplicates exist on the frontend we will have problems:

    1. Frontend adds URL breakpoint for "data"
      => Backend adds it
    2. Frontend adds second URL breakpoint for "data"
      => Backend does nothing (already in set)
    3. Frontend removes 1 of these
      => Backend removes it
    4. Frontend shows a Breakpoint but it isn't really working.

Seems we should prevent duplicates somewhere, perhaps in the frontend.

> Source/WebInspectorUI/UserInterface/Views/XHRBreakpointTreeElement.js:37
> +            title = WebInspector.UIString("URL contains:");

This is a rather wide string for every item, especially if users will likely contain a lot of text. I think just "URL: " would work. We can iterate on this.

[Joseph Pecoraro]

Comment on attachment 303967 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303967&action=review

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1140
> +        let url = popover.value;
> +        console.assert(url);

Is this assert valid? Can't the user put an empty value in the popover? We should just return and not create one with this special value (which means All Breakpoints).

[Matt Baker]

Committed r213691: https://trac.webkit.org/changeset/213691.