------- Inspected URL: https://devinrousso.com/demo/WebKit/test.html Loading completed: true Frontend User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko) Uncaught Exceptions: - TypeError: pauseReasonBreakpointTreeElement.removeStatusImage is not a function. (In 'pauseReasonBreakpointTreeElement.removeStatusImage()', 'pauseReasonBreakpointTreeElement.removeStatusImage' is undefined) (at DebuggerSidebarPanel.js:524:67) _removeBreakpoint @ DebuggerSidebarPanel.js:524:67 _breakpointRemoved @ DebuggerSidebarPanel.js:722:31 dispatch @ Object.js:170:30 dispatchEventToListeners @ Object.js:177:17 removeEventBreakpoint @ DOMDebuggerManager.js:259:38 ondelete @ EventBreakpointTreeElement.js:97:56 _treeKeyDown @ TreeOutline.js:579:60 _treeKeyDown @ [native code] ------- * STEPS TO REPRODUCE 1. Create a "click" event breakpoint 2. Click one of the "test" buttons in the page 3. Delete the "click" breakpoint in the Debugger sidebar
Created attachment 351463 [details] Patch
Isn't the real issue here that EventBreakpointTreeElement doesn't inherit from BreakpointTreeElement? Why not? If the change introduced by this patch is indeed the correct thing to do, an explanation in the change log would help.
Comment on attachment 351463 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351463&action=review > Source/WebInspectorUI/ChangeLog:10 > + Leverage existing `status` getter/setter for creating/removing the breakpoint icon element. I don't disagree with the spirit of this refactoring, but it just creates code churn and doesn't seem necessary to the fix.
(In reply to Matt Baker from comment #2) > Isn't the real issue here that EventBreakpointTreeElement doesn't inherit > from BreakpointTreeElement? Why not? If the change introduced by this patch > is indeed the correct thing to do, an explanation in the change log would > help. Alright I see that DOMBreakpointTreeElement and XHRBreakpointTreeElement also inherit from GeneralTreeElement, not BreakpointTreeElement.
Comment on attachment 351463 [details] Patch r-, since a little investigating revealed the problem is with our wacky tree element class hierarchy, not event breakpoints specifically. The bug also reproduces with DOM breakpoints and (I assume) XHR breakpoints. I think the correct fix should resolve the general problem affecting all three.
Comment on attachment 351463 [details] Patch (In reply to Matt Baker from comment #5) > Comment on attachment 351463 [details] > Patch > > r-, since a little investigating revealed the problem is with our wacky tree > element class hierarchy, not event breakpoints specifically. The bug also > reproduces with DOM breakpoints and (I assume) XHR breakpoints. I think the > correct fix should resolve the general problem affecting all three. Instead of creating `removeStatusImage` functions on DOM, Event, and XHR breakpoint tree elements, I tried to find a solution that was already present for all three. They all use `status` as the holder of the breakpoint icon, so removing it there makes the most sense. I feel like this is a simpler solution than trying to create a base class for breakpoint tree elements (not to mention the name `WI.BreakpointTreeElement` already exists).
Comment on attachment 351463 [details] Patch r=me
Comment on attachment 351463 [details] Patch Clearing flags on attachment: 351463 Committed r236848: <https://trac.webkit.org/changeset/236848>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45021035>