Bug 190230 – Web Inspector: REGRESSION(r236540): Uncaught Exception: TypeError: pauseReasonBreakpointTreeElement.removeStatusImage is not a function.

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

[Devin Rousso]

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

[Devin Rousso]

Created attachment 351463 [details]
Patch

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

(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.

[Matt Baker]

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.

[Devin Rousso]

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).

[Matt Baker]

Comment on attachment 351463 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 351463 [details]
Patch

Clearing flags on attachment: 351463

Committed r236848: <https://trac.webkit.org/changeset/236848>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45021035>