Bug 170916 - Web Inspector: Enabled state of "All Requests" XHR breakpoint not restored correctly
Summary: Web Inspector: Enabled state of "All Requests" XHR breakpoint not restored co...
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: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-17 14:26 PDT by Matt Baker
Modified: 2017-04-17 15:15 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2017-04-17 14:28 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-04-17 14:26:58 PDT
Summary:
Enabled state of "All Requests" XHR breakpoint not restored correctly. After creating the global breakpoint from the setting value, it should be set on the backend if enabled.

Right now the breakpoint has to be unchecked/checked for the setting to stick.

Steps to Reproduce:
1. Open a page using XHRs
2. Enable the All Requests breakpoint
3. Initiate an XHR
  => Pause
4. Reload Inspector
5. Initiate an XHR
  => No pause
Comment 1 Matt Baker 2017-04-17 14:28:11 PDT
Created attachment 307299 [details]
Patch
Comment 2 Joseph Pecoraro 2017-04-17 14:30:17 PDT
Comment on attachment 307299 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:71
> +            if (!this._allRequestsBreakpoint.disabled)
> +                this._updateXHRBreakpoint(this._allRequestsBreakpoint);

r- I do not think this makes sense in DOMDebuggerManager.

Workers can have "XHR"Breakpoints but they cannot have "DOM"Breakpoints. So I'd expect this in a location that would be compatible with workers.
Comment 3 Joseph Pecoraro 2017-04-17 14:32:14 PDT
Comment on attachment 307299 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:71
>> +                this._updateXHRBreakpoint(this._allRequestsBreakpoint);
> 
> r- I do not think this makes sense in DOMDebuggerManager.
> 
> Workers can have "XHR"Breakpoints but they cannot have "DOM"Breakpoints. So I'd expect this in a location that would be compatible with workers.

Ahh, okay "DOMDebugger" is what holds all of the Web specific breakpoints (DOM, XHR, eventually Event, Timeouts, etc). I was mistaken.

r=me
Comment 4 Radar WebKit Bug Importer 2017-04-17 14:35:24 PDT
<rdar://problem/31666196>
Comment 5 WebKit Commit Bot 2017-04-17 15:15:47 PDT
Comment on attachment 307299 [details]
Patch

Clearing flags on attachment: 307299

Committed r215435: <http://trac.webkit.org/changeset/215435>
Comment 6 WebKit Commit Bot 2017-04-17 15:15:49 PDT
All reviewed patches have been landed.  Closing bug.