Bug 183138 - Web Inspector: allow breakpoints to be set for specific event listeners
Summary: Web Inspector: allow breakpoints to be set for specific event listeners
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:
Keywords: InRadar
: 116405 (view as bug list)
Depends on: 183118
Blocks: 188778
  Show dependency treegraph
 
Reported: 2018-02-26 12:03 PST by Devin Rousso
Modified: 2018-09-05 09:45 PDT (History)
12 users (show)

See Also:


Attachments
Patch (99.80 KB, patch)
2018-02-26 22:11 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (117.09 KB, image/png)
2018-02-26 22:12 PST, Devin Rousso
no flags Details
Patch (54.97 KB, patch)
2018-08-16 12:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (55.86 KB, patch)
2018-08-17 13:59 PDT, Devin Rousso
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (3.10 MB, application/zip)
2018-08-17 16:01 PDT, EWS Watchlist
no flags Details
Patch (55.86 KB, patch)
2018-08-17 16:10 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (3.33 MB, application/zip)
2018-08-17 17:51 PDT, EWS Watchlist
no flags Details
Patch (56.15 KB, patch)
2018-08-17 21:30 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (56.47 KB, patch)
2018-08-17 22:44 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.42 MB, application/zip)
2018-08-17 23:32 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.79 MB, application/zip)
2018-08-18 00:02 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-sierra (3.11 MB, application/zip)
2018-08-18 00:39 PDT, EWS Watchlist
no flags Details
Patch (55.90 KB, patch)
2018-08-18 10:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (3.09 MB, application/zip)
2018-08-18 12:46 PDT, EWS Watchlist
no flags Details
Patch (55.67 KB, patch)
2018-08-20 11:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (59.32 KB, patch)
2018-08-20 15:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (59.15 KB, patch)
2018-08-20 16:54 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-02-26 12:03:33 PST
In addition to breaking on all "click" listeners, we should provide the ability to break on a specific event listener.
Comment 1 Devin Rousso 2018-02-26 22:11:29 PST
Created attachment 334671 [details]
Patch
Comment 2 Devin Rousso 2018-02-26 22:12:55 PST
Created attachment 334672 [details]
[Image] After Patch is applied
Comment 3 Joseph Pecoraro 2018-07-30 15:19:21 PDT
Comment on attachment 334671 [details]
Patch

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

The patch itself seems to have been just a WIP, but I threw some comments on anyways, since this is a natural progression after Event Breakpoints.

> Source/WebCore/ChangeLog:44
> +2018-02-26  Devin Rousso  <webkit@devinrousso.com>

Nit: Double changelog.

> Source/WebInspectorUI/ChangeLog:31
> +2018-02-26  Devin Rousso  <webkit@devinrousso.com>

Nit: Double changelog.

> Source/JavaScriptCore/inspector/protocol/DOM.json:278
> +            "name": "setBreakpointForEventListener",

Note: We will want to prepare an internal change for these protocol modifications. So we'll want to do a coordinated landing.

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:287
> +        RefPtr<EventListener> eventListener;

Does this now need to be cleared across page loads? Maybe it already does, I just want to make sure.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:147
> +localizedStrings["Break on listeners for event with name:"] = "Break on listeners for event with name:";

Most of the frontend changes are duplicate of the other patch (I guess the double ChangeLogs should have made that obvious). So I'll hav limited frontend comments.
Comment 4 Devin Rousso 2018-08-16 12:08:45 PDT
Created attachment 347283 [details]
Patch
Comment 5 Devin Rousso 2018-08-17 13:59:19 PDT
Created attachment 347388 [details]
Patch

Rebase
Comment 6 EWS Watchlist 2018-08-17 16:01:26 PDT Comment hidden (obsolete)
Comment 7 EWS Watchlist 2018-08-17 16:01:27 PDT Comment hidden (obsolete)
Comment 8 Devin Rousso 2018-08-17 16:10:16 PDT
Created attachment 347407 [details]
Patch

Retry EWS
Comment 9 EWS Watchlist 2018-08-17 17:51:18 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-08-17 17:51:19 PDT Comment hidden (obsolete)
Comment 11 Devin Rousso 2018-08-17 21:30:55 PDT
Created attachment 347428 [details]
Patch

Try again ... not sure what's wrong
Comment 12 Devin Rousso 2018-08-17 22:44:53 PDT
Created attachment 347432 [details]
Patch

Extra logging for EWS
Comment 13 EWS Watchlist 2018-08-17 23:32:45 PDT Comment hidden (obsolete)
Comment 14 EWS Watchlist 2018-08-17 23:32:47 PDT Comment hidden (obsolete)
Comment 15 EWS Watchlist 2018-08-18 00:02:50 PDT Comment hidden (obsolete)
Comment 16 EWS Watchlist 2018-08-18 00:02:52 PDT Comment hidden (obsolete)
Comment 17 EWS Watchlist 2018-08-18 00:39:05 PDT Comment hidden (obsolete)
Comment 18 EWS Watchlist 2018-08-18 00:39:07 PDT Comment hidden (obsolete)
Comment 19 Devin Rousso 2018-08-18 10:53:11 PDT
Created attachment 347446 [details]
Patch

My guess is that by calling `clear()` we are deleting `m_table`, which prevents our iteration
Comment 20 EWS Watchlist 2018-08-18 12:46:57 PDT Comment hidden (obsolete)
Comment 21 EWS Watchlist 2018-08-18 12:46:58 PDT Comment hidden (obsolete)
Comment 22 Devin Rousso 2018-08-20 11:58:15 PDT
Created attachment 347521 [details]
Patch

Mutation during Iteration 😅
Comment 23 Joseph Pecoraro 2018-08-20 14:08:46 PDT
Comment on attachment 347521 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/protocol/DOM.json:94
> +                { "name": "hasBreakpoint", "type": "boolean", "optional": true }

Offline: These will require a slight internal change so hold off on landing until we talk.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:847
> +        for (auto& inspectorEventListener : m_eventListenerEntries.values()) {
> +            if (inspectorEventListener.matches(*info.node, info.eventType, listener.callback(), listener.useCapture())) {

Do you want to break inside this if to avoid looping over times after you've found a match?

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:305
> +            return eventTarget.get() == &target && eventListener.get() == &listener && eventType == type && useCapture == capture;

Style: Lets break each condition into its own line to make this a little more readable.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:369
> +    bool shouldPause = m_debuggerAgent->pauseOnNextStatementEnabled() || m_eventListenerBreakpoints.contains(listenerEventCategoryType + event.type());

Should we rename "listenerEventCategoryType" from "listener:" to "eventNameCategoryType" with a value of "event-name:"?

I found it confusing to distinction between actual event listener breakpoints and event name breakpoints, especially given the poorly named `InspectorDOMDebuggerAgent::setEventListenerBreakpoint` which is event name breakpoints and we are just keeping the legacy name for backwards compatibility.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:383
> +    if (m_domAgent) {
> +        int eventListenerId = m_domAgent->idForEventListener(*event.currentTarget(), event.type(), registeredEventListener.callback(), registeredEventListener.useCapture());
> +        if (eventListenerId)
> +            eventData->setInteger("eventListenerId"_s, eventListenerId);
> +    }

Its a shame that we are doing potentially 2 O(n) iterations to lookup hasBreakpoints and id for the EventListener, but I think we can assume that the number of event listener breakpoints is pretty small.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:53
> +    get eventBreakpoints()

Maybe we name these `eventListenerBreakpoints` so that it doesn't get confused with `eventBreakpoints` on DOMDebuggerManager? I realize that the type are WI.EventBreakpoint.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1078
> +                let eventListener = eventBreakpoint.eventListener;
> +                if (eventListener && eventListener.eventListenerId === pauseData.eventListenerId) {

Would it be safe to drop this 2nd check, or just make it an assert?

    if (eventListener) {
        console.assert(eventListener.eventListenerId === pauseData.eventListenerId);
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:59
> +            if (DOMAgent.setBreakpointForEventListener && DOMAgent.removeBreakpointForEventListener)

Good feature detection 👍!

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:47
> +                if (paused)
> +                    InspectorTest.fail("Should not pause twice.");

I don't think this can be reached given `singleFireEventListener` above. So you could probably drop this if you wanted.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:53
> +                InspectorTest.assert(targetData.pauseReason === WI.DebuggerManager.PauseReason.EventListener, `Pause reason should be EventListener.`);
> +                InspectorTest.assert(targetData.pauseData.eventName === "click", `Pause data eventName should be "click".`);

You could make these InspectorTest.expectThat(..., `...`) so these show up in the output as PASS / FAIL lines.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:112
> +                if (paused)
> +                    InspectorTest.fail("Should not pause twice.");

Ditto.
Comment 24 Devin Rousso 2018-08-20 14:36:38 PDT
Comment on attachment 347521 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:847
>> +            if (inspectorEventListener.matches(*info.node, info.eventType, listener.callback(), listener.useCapture())) {
> 
> Do you want to break inside this if to avoid looping over times after you've found a match?

I don't think it's possible to have more than one, and even if it was, there's nothing we do about that, so yeah.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:369
>> +    bool shouldPause = m_debuggerAgent->pauseOnNextStatementEnabled() || m_eventListenerBreakpoints.contains(listenerEventCategoryType + event.type());
> 
> Should we rename "listenerEventCategoryType" from "listener:" to "eventNameCategoryType" with a value of "event-name:"?
> 
> I found it confusing to distinction between actual event listener breakpoints and event name breakpoints, especially given the poorly named `InspectorDOMDebuggerAgent::setEventListenerBreakpoint` which is event name breakpoints and we are just keeping the legacy name for backwards compatibility.

I think we should also change `instrumentationEventCategoryType` to just `instrumentationCategoryType`, since instrumentation "event"s aren't actually events.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:383
>> +    }
> 
> Its a shame that we are doing potentially 2 O(n) iterations to lookup hasBreakpoints and id for the EventListener, but I think we can assume that the number of event listener breakpoints is pretty small.

Not only that, but this is only based on the number of indexed/tracked event listeners, so it is even further limited by the number of event listeners sent to the frontend.  We only generate event listener IDs when `DOM.getEventListenersForNode` is called.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:53
>> +    get eventBreakpoints()
> 
> Maybe we name these `eventListenerBreakpoints` so that it doesn't get confused with `eventBreakpoints` on DOMDebuggerManager? I realize that the type are WI.EventBreakpoint.

Agreed.
Comment 25 Devin Rousso 2018-08-20 15:54:30 PDT
Created attachment 347554 [details]
Patch
Comment 26 Devin Rousso 2018-08-20 16:54:03 PDT
Created attachment 347573 [details]
Patch

Somehow missed my previous mutation during iteration change
Comment 27 WebKit Commit Bot 2018-08-20 19:15:34 PDT
Comment on attachment 347573 [details]
Patch

Clearing flags on attachment: 347573

Committed r235103: <https://trac.webkit.org/changeset/235103>
Comment 28 WebKit Commit Bot 2018-08-20 19:15:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Radar WebKit Bug Importer 2018-08-20 19:16:20 PDT
<rdar://problem/43540620>
Comment 30 Devin Rousso 2018-09-05 09:45:24 PDT
*** Bug 116405 has been marked as a duplicate of this bug. ***