Bug 190230 - Web Inspector: REGRESSION(r236540): Uncaught Exception: TypeError: pauseReasonBreakpointTreeElement.removeStatusImage is not a function.
Summary: Web Inspector: REGRESSION(r236540): Uncaught Exception: TypeError: pauseReaso...
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: Devin Rousso
URL: https://devinrousso.com/demo/WebKit/t...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-02 18:04 PDT by Devin Rousso
Modified: 2018-10-04 14:01 PDT (History)
4 users (show)

See Also:


Attachments
Patch (15.88 KB, patch)
2018-10-02 18:20 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-10-02 18:04:27 PDT
-------
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
Comment 1 Devin Rousso 2018-10-02 18:20:58 PDT
Created attachment 351463 [details]
Patch
Comment 2 Matt Baker 2018-10-04 12:10:05 PDT
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 3 Matt Baker 2018-10-04 12:11:42 PDT
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.
Comment 4 Matt Baker 2018-10-04 12:13:10 PDT
(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 5 Matt Baker 2018-10-04 12:17:40 PDT
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 6 Devin Rousso 2018-10-04 12:36:26 PDT
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 7 Matt Baker 2018-10-04 13:32:15 PDT
Comment on attachment 351463 [details]
Patch

r=me
Comment 8 WebKit Commit Bot 2018-10-04 13:58:24 PDT
Comment on attachment 351463 [details]
Patch

Clearing flags on attachment: 351463

Committed r236848: <https://trac.webkit.org/changeset/236848>
Comment 9 WebKit Commit Bot 2018-10-04 13:58:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-10-04 14:01:08 PDT
<rdar://problem/45021035>