Bug 170033 - Web Inspector: XHR breakpoints should be global
Summary: Web Inspector: XHR breakpoints should be global
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: Matt Baker
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-23 16:26 PDT by Matt Baker
Modified: 2017-04-05 11:57 PDT (History)
5 users (show)

See Also:


Attachments
[Image] new tree element titles (100.73 KB, image/png)
2017-03-23 16:26 PDT, Matt Baker
no flags Details
Patch (13.80 KB, patch)
2017-03-24 10:54 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1020.42 KB, application/zip)
2017-03-24 11:41 PDT, Build Bot
no flags Details
Patch (13.93 KB, patch)
2017-03-25 09:44 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (831.47 KB, application/zip)
2017-03-25 10:51 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.07 MB, application/zip)
2017-03-25 11:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-elcapitan (1.66 MB, application/zip)
2017-03-25 11:05 PDT, Build Bot
no flags Details
Patch (14.51 KB, patch)
2017-03-25 11:06 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (907.60 KB, application/zip)
2017-03-25 12:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.04 MB, application/zip)
2017-03-25 12:18 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.68 MB, application/zip)
2017-03-25 12:29 PDT, Build Bot
no flags Details
Patch (16.08 KB, patch)
2017-03-25 13:42 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (999.49 KB, application/zip)
2017-03-25 20:58 PDT, Build Bot
no flags Details
Patch (16.04 KB, patch)
2017-03-26 08:21 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (980.55 KB, application/zip)
2017-03-27 06:29 PDT, Build Bot
no flags Details
Patch for landing (16.14 KB, patch)
2017-03-30 13:03 PDT, Matt Baker
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 2017-03-23 16:26:54 PDT
Created attachment 305238 [details]
[Image] new tree element titles

Summary:
XHR breakpoints should be global. They are currently associated with the main frame URL.

Note:
Tree element titles for XHR breakpoints should "URL — <url text>", instead of "URL contains: <url text>". See screenshot.
Comment 1 Matt Baker 2017-03-24 10:54:20 PDT
Created attachment 305297 [details]
Patch
Comment 2 Build Bot 2017-03-24 11:40:59 PDT
Comment on attachment 305297 [details]
Patch

Attachment 305297 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3404861

New failing tests:
inspector/debugger/tail-deleted-frames.html
inspector/debugger/probe-manager-add-remove-actions.html
inspector/debugger/tail-recursion.html
inspector/debugger/breakpoint-columns.html
inspector/debugger/tail-deleted-frames-this-value.html
http/tests/inspector/network/resource-response-source-memory-cache.html
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/dom-debugger/xhr-breakpoints.html
inspector/debugger/search-scripts.html
inspector/debugger/breakpoint-scope.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/console/messagesCleared.html
inspector/debugger/debugger-stack-overflow.html
Comment 3 Build Bot 2017-03-24 11:41:01 PDT
Created attachment 305307 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Joseph Pecoraro 2017-03-24 11:48:36 PDT
Comment on attachment 305297 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:186
> +        if (this._xhrBreakpoints.some((entry) => entry.url === breakpoint.url))
> +            return;

How does the UI handle this? What does the UI display when the user adds the same URL breakpoint twice? It looks like the UI would show it twice, until we navigate and then it would show 1. That might need to be improved.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:363
> +            breakpoint.dispatchEventToListeners(WebInspector.XHRBreakpoint.Event.XHRBreakpointResolved);

My checkout doesn't have this event name. What is this? r- for this

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:112
>  .sidebar > .panel.navigation.debugger .details-section.xhr-breakpoints .item.breakpoint .subtitle {
> -    padding-left: 5px;
>      font-family: Menlo, monospace;

Food for thought: If we supported regular expressions (which I think we should because it would be a very small patch) we could use a FormattedValue here. We could show:

    URL – "foo"
    URL – /foo\d/

or:

    URL contains: "foo"
    URL matches: /foo\d/

And they would be syntax highlighted like they are in the console. What you have now looks good to me, far more compact.
Comment 5 Matt Baker 2017-03-25 09:38:17 PDT
Comment on attachment 305297 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:186
>> +            return;
> 
> How does the UI handle this? What does the UI display when the user adds the same URL breakpoint twice? It looks like the UI would show it twice, until we navigate and then it would show 1. That might need to be improved.

The ".some(() => ...)" test prevents this from happening. No message is shown to the user when attempting to add an XHR breakpoint with a duplicate.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:363
>> +            breakpoint.dispatchEventToListeners(WebInspector.XHRBreakpoint.Event.XHRBreakpointResolved);
> 
> My checkout doesn't have this event name. What is this? r- for this

This is a typo. Fixed.

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.css:112
>>      font-family: Menlo, monospace;
> 
> Food for thought: If we supported regular expressions (which I think we should because it would be a very small patch) we could use a FormattedValue here. We could show:
> 
>     URL – "foo"
>     URL – /foo\d/
> 
> or:
> 
>     URL contains: "foo"
>     URL matches: /foo\d/
> 
> And they would be syntax highlighted like they are in the console. What you have now looks good to me, far more compact.

This is exactly what I'd like to do! We should also use a CodeMirror instance in InputPopover to take advantage of CodeMirror's regex parsing/highlighting. Filed a follow-up:

Web Inspector: Add regular expression support to XHR breakpoints
https://bugs.webkit.org/show_bug.cgi?id=170099
Comment 6 Matt Baker 2017-03-25 09:44:33 PDT
Created attachment 305378 [details]
Patch
Comment 7 Build Bot 2017-03-25 10:51:40 PDT
Comment on attachment 305378 [details]
Patch

Attachment 305378 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3409890

New failing tests:
inspector/debugger/tail-deleted-frames.html
inspector/debugger/probe-manager-add-remove-actions.html
inspector/debugger/tail-recursion.html
inspector/debugger/breakpoint-columns.html
inspector/debugger/tail-deleted-frames-this-value.html
http/tests/inspector/network/resource-response-source-memory-cache.html
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/dom-debugger/xhr-breakpoints.html
inspector/debugger/search-scripts.html
inspector/debugger/breakpoint-scope.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/console/messagesCleared.html
inspector/debugger/debugger-stack-overflow.html
Comment 8 Build Bot 2017-03-25 10:51:42 PDT
Created attachment 305383 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 9 Build Bot 2017-03-25 11:00:49 PDT
Comment on attachment 305378 [details]
Patch

Attachment 305378 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3409900

New failing tests:
inspector/debugger/tail-deleted-frames.html
inspector/debugger/probe-manager-add-remove-actions.html
inspector/debugger/tail-recursion.html
inspector/debugger/breakpoint-columns.html
inspector/debugger/tail-deleted-frames-this-value.html
http/tests/inspector/network/resource-response-source-memory-cache.html
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/dom-debugger/xhr-breakpoints.html
inspector/debugger/search-scripts.html
inspector/debugger/breakpoint-scope.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
inspector/console/messagesCleared.html
inspector/debugger/debugger-stack-overflow.html
Comment 10 Build Bot 2017-03-25 11:00:52 PDT
Created attachment 305384 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 11 Build Bot 2017-03-25 11:05:11 PDT
Comment on attachment 305378 [details]
Patch

Attachment 305378 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3409894

New failing tests:
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/probe-manager-add-remove-actions.html
inspector/debugger/tail-recursion.html
inspector/debugger/tail-deleted-frames-this-value.html
http/tests/inspector/network/resource-response-source-memory-cache.html
inspector/debugger/tail-deleted-frames.html
inspector/dom-debugger/xhr-breakpoints.html
http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html
Comment 12 Build Bot 2017-03-25 11:05:14 PDT
Created attachment 305385 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 13 Matt Baker 2017-03-25 11:06:01 PDT
Created attachment 305386 [details]
Patch
Comment 14 Build Bot 2017-03-25 12:13:18 PDT
Comment on attachment 305386 [details]
Patch

Attachment 305386 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3410131

New failing tests:
inspector/dom-debugger/xhr-breakpoints.html
Comment 15 Build Bot 2017-03-25 12:13:21 PDT
Created attachment 305390 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 16 Build Bot 2017-03-25 12:18:02 PDT
Comment on attachment 305386 [details]
Patch

Attachment 305386 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3410135

New failing tests:
inspector/dom-debugger/xhr-breakpoints.html
Comment 17 Build Bot 2017-03-25 12:18:04 PDT
Created attachment 305391 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 18 Build Bot 2017-03-25 12:29:12 PDT
Comment on attachment 305386 [details]
Patch

Attachment 305386 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3410132

New failing tests:
inspector/dom-debugger/xhr-breakpoints.html
Comment 19 Build Bot 2017-03-25 12:29:14 PDT
Created attachment 305392 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 20 Matt Baker 2017-03-25 13:42:17 PDT
Created attachment 305396 [details]
Patch
Comment 21 Build Bot 2017-03-25 20:58:14 PDT
Comment on attachment 305396 [details]
Patch

Attachment 305396 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3411157

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html
Comment 22 Build Bot 2017-03-25 20:58:17 PDT
Created attachment 305413 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 23 Matt Baker 2017-03-26 08:21:51 PDT
Created attachment 305420 [details]
Patch
Comment 24 Build Bot 2017-03-27 06:29:54 PDT
Comment on attachment 305420 [details]
Patch

Attachment 305420 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3417159

New failing tests:
compositing/absolute-inside-out-of-view-fixed.html
Comment 25 Build Bot 2017-03-27 06:29:57 PDT
Created attachment 305464 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 26 Joseph Pecoraro 2017-03-27 13:14:36 PDT
Comment on attachment 305297 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:186
>>> +            return;
>> 
>> How does the UI handle this? What does the UI display when the user adds the same URL breakpoint twice? It looks like the UI would show it twice, until we navigate and then it would show 1. That might need to be improved.
> 
> The ".some(() => ...)" test prevents this from happening. No message is shown to the user when attempting to add an XHR breakpoint with a duplicate.

Oh, so the user attempts to add a breakpoint but no new breakpoint shows up because one already exists with the same URL. Okay, that seems fine.
Comment 27 Joseph Pecoraro 2017-03-27 14:54:28 PDT
Comment on attachment 305420 [details]
Patch

r=me
Comment 28 Matt Baker 2017-03-30 13:03:07 PDT
Created attachment 305883 [details]
Patch for landing
Comment 29 WebKit Commit Bot 2017-04-05 11:57:38 PDT
Comment on attachment 305883 [details]
Patch for landing

Clearing flags on attachment: 305883

Committed r214956: <http://trac.webkit.org/changeset/214956>
Comment 30 WebKit Commit Bot 2017-04-05 11:57:40 PDT
All reviewed patches have been landed.  Closing bug.