Bug 215362 – Web Inspector: allow event breakpoints to be configured

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

[Devin Rousso]

This would allow developers to do things like:
 - only pause when `window.event.type` is a certain value
 - ignore the first N pauses
 - evaluate JavaScript whenever an event listener is invoked without pausing

[Devin Rousso]

Created attachment 406370 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Darin Adler]

Comment on attachment 406370 [details]
Patch

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

> Source/JavaScriptCore/debugger/Breakpoint.h:45
> +    class Action {

Can this be a struct instead of a class? Doesn't seem like it needs to be a class.

> Source/JavaScriptCore/debugger/Breakpoint.h:53
> +        enum class Type {
> +            Log,
> +            Evaluate,
> +            Sound,
> +            Probe,
> +        };

I would do this as one-liner and use a backing type:

    enum class Type : uint8_t { Log, Evaluate, Sound, Probe };

> Source/JavaScriptCore/debugger/Breakpoint.h:87
> +    bool autoContinue() const { return m_autoContinue; }

This seems like one of these names where we want to use "is" predicate naming so it doesn’t sound like we are telling the object to "auto continue".

isAutoContinue

> Source/JavaScriptCore/debugger/Breakpoint.h:92
> +    bool linked() const { return m_sourceID != noSourceID; }

isLinked, so we know we are not telling this object, "this is now linked"

> Source/JavaScriptCore/debugger/Breakpoint.h:94
> +    bool resolved() const { return m_resolved; }

isResolved, so we know we are not telling this object, "this is now resolved"

> Source/JavaScriptCore/debugger/Breakpoint.h:120
> +    bool m_autoContinue;

Might want to initialize this here even if the constructor already does.

[Devin Rousso]

Comment on attachment 406370 [details]
Patch

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

>> Source/JavaScriptCore/debugger/Breakpoint.h:45
>> +    class Action {
> 
> Can this be a struct instead of a class? Doesn't seem like it needs to be a class.

My main goal behind this was to have something "constant after creation", at least for all external callers.  Whether it's a `struct` or `class` I don't have a preference, but I do like and want the fact that it's basically readonly.

>> Source/JavaScriptCore/debugger/Breakpoint.h:53
>> +        };
> 
> I would do this as one-liner and use a backing type:
> 
>     enum class Type : uint8_t { Log, Evaluate, Sound, Probe };

I prefer putting these on separate lines as I find that easier to read (rather than "hunting" for commas) and is more nicely diff-able.  If you feel strongly about this (or it's in the style guide) tho, I can change it.

I'll add a backing type.

>> Source/JavaScriptCore/debugger/Breakpoint.h:87
>> +    bool autoContinue() const { return m_autoContinue; }
> 
> This seems like one of these names where we want to use "is" predicate naming so it doesn’t sound like we are telling the object to "auto continue".
> 
> isAutoContinue

Ah good point.  I'll change it.

>> Source/JavaScriptCore/debugger/Breakpoint.h:92
>> +    bool linked() const { return m_sourceID != noSourceID; }
> 
> isLinked, so we know we are not telling this object, "this is now linked"

👍

>> Source/JavaScriptCore/debugger/Breakpoint.h:94
>> +    bool resolved() const { return m_resolved; }
> 
> isResolved, so we know we are not telling this object, "this is now resolved"

👍

>> Source/JavaScriptCore/debugger/Breakpoint.h:120
>> +    bool m_autoContinue;
> 
> Might want to initialize this here even if the constructor already does.

yeah I suppose that's fine (`m_ignoreCount` and `m_id` too)

[Darin Adler]

Comment on attachment 406370 [details]
Patch

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

>>> Source/JavaScriptCore/debugger/Breakpoint.h:45
>>> +    class Action {
>> 
>> Can this be a struct instead of a class? Doesn't seem like it needs to be a class.
> 
> My main goal behind this was to have something "constant after creation", at least for all external callers.  Whether it's a `struct` or `class` I don't have a preference, but I do like and want the fact that it's basically readonly.

Using "struct Action" and then "const Vector<Action>" or "Vector<const Action>" accomplishes this in a really clear way, without requiring you write a constructor and getter functions for each data member.

[Devin Rousso]

Comment on attachment 406370 [details]
Patch

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

>>>> Source/JavaScriptCore/debugger/Breakpoint.h:45
>>>> +    class Action {
>>> 
>>> Can this be a struct instead of a class? Doesn't seem like it needs to be a class.
>> 
>> My main goal behind this was to have something "constant after creation", at least for all external callers.  Whether it's a `struct` or `class` I don't have a preference, but I do like and want the fact that it's basically readonly.
> 
> Using "struct Action" and then "const Vector<Action>" or "Vector<const Action>" accomplishes this in a really clear way, without requiring you write a constructor and getter functions for each data member.

I'd originally tried using `const Action` everywhere, but that wasn't working :/

Perhaps I just messed something up at the time.  I'll give it another shot.

[Darin Adler]

Comment on attachment 406370 [details]
Patch

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

>>> Source/JavaScriptCore/debugger/Breakpoint.h:53
>>> +        };
>> 
>> I would do this as one-liner and use a backing type:
>> 
>>     enum class Type : uint8_t { Log, Evaluate, Sound, Probe };
> 
> I prefer putting these on separate lines as I find that easier to read (rather than "hunting" for commas) and is more nicely diff-able.  If you feel strongly about this (or it's in the style guide) tho, I can change it.
> 
> I'll add a backing type.

The tall format doesn’t make it easier for *me* to see a list of a few values. It’s required when the list is long, but for 4 short words I like the single line way better.

If "hunting for commas" was a problem we’d probably need to write all our function call argument lists vertically too.

This is a matter of taste, and not a project guideline.

> Source/JavaScriptCore/debugger/Breakpoint.h:108
> +    BreakpointID m_id;

Oh, I meant to say the same thing about this (initialize even though the constructor does it too).

[Darin Adler]

Comment on attachment 406370 [details]
Patch

Not really reviewing for now until we resolve the Windows build failure issue.

[Darin Adler]

Comment on attachment 406370 [details]
Patch

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

> Source/JavaScriptCore/debugger/Breakpoint.h:72
> +    using ActionsList = Vector<Action>;

"list" is not a great term for a vector

> Source/JavaScriptCore/debugger/Breakpoint.h:86
> +    const ActionsList& actions() const { return m_actions; }

This "const" should be all we need. The Action returned from such a vector will be const.

> Source/JavaScriptCore/debugger/DebuggerPrimitives.h:38
> +typedef int BreakpointActionID;

New code should use "using", not "typedef".

> Source/JavaScriptCore/debugger/DebuggerPrimitives.h:39
> +static constexpr BreakpointActionID noBreakpointActionID = 0;

The use of "static" here (and above) is not right; it gives these "internal linkage" but that’s not desirable in a header. Should just say constexpr and not static since this is a header.

[Devin Rousso]

Created attachment 406443 [details]
Patch

Address @Darin Adler's feedback.

Attempt to fix the wincairo/win build using @Fujii Hironori's suggestion.

[Devin Rousso]

Created attachment 406444 [details]
Patch

fix debug build

[Radar WebKit Bug Importer]

<rdar://problem/66932921>

[Blaze Burg]

Comment on attachment 406444 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:131
> +        `ProtocolBreakpoint` is a sort of "blueprint" that is actualized whenever a new script is

Nit: template

> Source/JavaScriptCore/debugger/Breakpoint.cpp:52
> +bool Breakpoint::link(SourceID sourceID, unsigned lineNumber, unsigned columnNumber)

Can you drop a note in this class to remind how special breakpoints are encoded? I believe it's using a dummy sourceID?

> Source/JavaScriptCore/debugger/Debugger.cpp:425
> +    DebuggerParseData& parseData = debuggerParseData(breakpoint.sourceID(), sourceProvider);

I was expecting a higher-level explanation like "JSC's DebuggerParseData expects column offsets to be relative to the script source inside of the <script> tag, whereas Breakpoint's column offset is with respect to the entire file."

Similarly, in the header of Breakpoint class, a simple explanation would be helpful "linked means it's be associated with a source file or fake source file (special breakpoint), and resolved means that we validated the location based on parse data (possibly shifting line/col to the next real pause opportunity)."

> Source/JavaScriptCore/debugger/Debugger.cpp:446
> +    auto& breakpointsForLine = m_breakpointsForSourceID.ensure(breakpoint.sourceID(), [] {

v.nice to use ensure(), but it seems like the locals are named incorrectly?

The first one should be `breakpointsForSourceID` and the second should be `breakpointsForLine`. I think the rest of the logic related is correct as-is.

> Source/JavaScriptCore/debugger/Debugger.cpp:455
> +        if (existingBreakpoint->columnNumber() == breakpoint.columnNumber()) {

I haven't read the entire patch, but how do we ensure that editing a breakpoint condition will not try to set a second breakpoint on the same line? Breakpoint objects are immutable, so I'm guessing that they are added and removed somewhere upon update? Perhaps this didn't change in the patch, but I'm not familiar with the entire design.

> Source/JavaScriptCore/debugger/Debugger.cpp:475
> +    auto breakpointsForLineIterator = m_breakpointsForSourceID.find(breakpoint.sourceID());

Again, breakpointsForSourceID and breakpointsForLine would make more sense for local names here.

> Source/JavaScriptCore/debugger/Debugger.cpp:517
> +    auto breakpointsForLineIterator = m_breakpointsForSourceID.find(sourceID);

Ditto the above comment about naming iterators.

> Source/JavaScriptCore/debugger/Debugger.h:154
> +        virtual void failedToParseSource(const String& /* url */, const String& /* data */, int /* firstLine */, int /* errorLine */, const String& /* errorMessage */) { }

Why comment out the parameter names? This seem unnecessary.

[Devin Rousso]

Comment on attachment 406444 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:131
>> +        `ProtocolBreakpoint` is a sort of "blueprint" that is actualized whenever a new script is
> 
> Nit: template

👍

>> Source/JavaScriptCore/debugger/Breakpoint.cpp:52
>> +bool Breakpoint::link(SourceID sourceID, unsigned lineNumber, unsigned columnNumber)
> 
> Can you drop a note in this class to remind how special breakpoints are encoded? I believe it's using a dummy sourceID?

Special breakpoints don't have any requirement about how they're encoded/linked/resolved/etc.  They're really just a way of telling `Debugger` "I want to you pause ASAP and use this 'special' breakpoint's condition/ignoreCount/actions/etc. (in addition to any existing breakpoint that may coincidentally exist at the same spot as the ASAP pause)".

Frankly, I'm open to the idea of using a different name other than "special", I just couldn't think of anything that was quite as ... special :|

>> Source/JavaScriptCore/debugger/Debugger.cpp:425
>> +    DebuggerParseData& parseData = debuggerParseData(breakpoint.sourceID(), sourceProvider);
> 
> I was expecting a higher-level explanation like "JSC's DebuggerParseData expects column offsets to be relative to the script source inside of the <script> tag, whereas Breakpoint's column offset is with respect to the entire file."
> 
> Similarly, in the header of Breakpoint class, a simple explanation would be helpful "linked means it's be associated with a source file or fake source file (special breakpoint), and resolved means that we validated the location based on parse data (possibly shifting line/col to the next real pause opportunity)."

Good idea.  I'll add comments in `Breakpoint.h`.

>> Source/JavaScriptCore/debugger/Debugger.cpp:446
>> +    auto& breakpointsForLine = m_breakpointsForSourceID.ensure(breakpoint.sourceID(), [] {
> 
> v.nice to use ensure(), but it seems like the locals are named incorrectly?
> 
> The first one should be `breakpointsForSourceID` and the second should be `breakpointsForLine`. I think the rest of the logic related is correct as-is.

I disagree.  The result of `m_breakpointsForSourceID.ensure` is a `LineToBreakpointsMap` (a.k.a `HashMap<unsigned, BreakpointsVector>`), which is a "map of breakpoints for each line number.  Similarly, the result of `breakpointsForLine.ensure` is a `BreakpointsVector` (which is manually uniqued with respect to column numbers).

>> Source/JavaScriptCore/debugger/Debugger.cpp:455
>> +        if (existingBreakpoint->columnNumber() == breakpoint.columnNumber()) {
> 
> I haven't read the entire patch, but how do we ensure that editing a breakpoint condition will not try to set a second breakpoint on the same line? Breakpoint objects are immutable, so I'm guessing that they are added and removed somewhere upon update? Perhaps this didn't change in the patch, but I'm not familiar with the entire design.

The frontend sends a remove and then re-adds after each edit.  See `WI.DebuggerManager.prototype._breakpointEditablePropertyDidChange`.  That definitely could be improved, but that's a separate issue.

>> Source/JavaScriptCore/debugger/Debugger.h:154
>> +        virtual void failedToParseSource(const String& /* url */, const String& /* data */, int /* firstLine */, int /* errorLine */, const String& /* errorMessage */) { }
> 
> Why comment out the parameter names? This seem unnecessary.

otherwise we'd get compile errors saying that the variable is unused, as this method is not a pure `virtual`

[Blaze Burg]

Comment on attachment 406444 [details]
Patch

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

>>> Source/JavaScriptCore/debugger/Debugger.cpp:446
>>> +    auto& breakpointsForLine = m_breakpointsForSourceID.ensure(breakpoint.sourceID(), [] {
>> 
>> v.nice to use ensure(), but it seems like the locals are named incorrectly?
>> 
>> The first one should be `breakpointsForSourceID` and the second should be `breakpointsForLine`. I think the rest of the logic related is correct as-is.
> 
> I disagree.  The result of `m_breakpointsForSourceID.ensure` is a `LineToBreakpointsMap` (a.k.a `HashMap<unsigned, BreakpointsVector>`), which is a "map of breakpoints for each line number.  Similarly, the result of `breakpointsForLine.ensure` is a `BreakpointsVector` (which is manually uniqued with respect to column numbers).

OK

>>> Source/JavaScriptCore/debugger/Debugger.cpp:455
>>> +        if (existingBreakpoint->columnNumber() == breakpoint.columnNumber()) {
>> 
>> I haven't read the entire patch, but how do we ensure that editing a breakpoint condition will not try to set a second breakpoint on the same line? Breakpoint objects are immutable, so I'm guessing that they are added and removed somewhere upon update? Perhaps this didn't change in the patch, but I'm not familiar with the entire design.
> 
> The frontend sends a remove and then re-adds after each edit.  See `WI.DebuggerManager.prototype._breakpointEditablePropertyDidChange`.  That definitely could be improved, but that's a separate issue.

OK

[Devin Rousso]

Created attachment 406643 [details]
Patch

added some comments addressing @Brian Burg's feedback

[Blaze Burg]

Comment on attachment 406643 [details]
Patch

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

> Source/JavaScriptCore/debugger/Debugger.cpp:908
>      if (blackboxTypeIterator != m_blackboxedScripts.end() && blackboxTypeIterator->value == BlackboxType::Deferred) {

Nit: it would be nice to turn this into an Optional<> instead of repeating the iterator checks throughout the function.

[Blaze Burg]

Comment on attachment 406643 [details]
Patch

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

> Source/JavaScriptCore/debugger/Debugger.h:210
> +    virtual void reportException(JSGlobalObject*, Exception*) const { }

The comment needs to move to the new declaration. I do think this state of affairs is kind of bad. When there is an exception in the condition or actions, it shows up as an error at the line for the breakpoint. I think it should just be reflected in the actions editor UI itself and mark the breakpoint as "failing" or something (red outline?). There are several bugs open such as https://bugs.webkit.org/show_bug.cgi?id=158817 which try to address this, but it's been subpar for a long time.

> Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.h:-56
> -    void reportException(JSC::JSGlobalObject*, JSC::Exception*) const final { }

The comment needs to move to the new declaration. I do think this state of affairs is kind of bad. When there is an exception in the condition or actions, it shows up as an error at the line for the breakpoint. I think it should just be reflected in the actions editor UI itself and mark the breakpoint as "failing" or something (red outline?).

There are several bugs open such as https://bugs.webkit.org/show_bug.cgi?id=158817 which try to address this, but it's been subpar for a long time.

[Blaze Burg]

Ugh, I hate bugzilla. Sorry about the double comment. I lost many other review comments due to the server timing out. Most of them were questions I had which were later answered by reading more of the patch.

[Blaze Burg]

Comment on attachment 406643 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1054
> +        if (!protocolBreakpoint.matches(scriptURLForBreakpoints))

This would read better as .matchesURL (it doesn't check line/column).

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1241
> +

Do we expect any more entries to remain in m_protocolBreakpointForProtocolBreakpointID at this point? removeBreakpoint() should have cleared the entries one by one.

> Source/JavaScriptCore/runtime/JSMicrotask.cpp:95
> +    if (UNLIKELY(globalObject->hasDebugger()))

It's worrying that this omission wasn't caught by a test. Can you fix that?

[Blaze Burg]

Comment on attachment 406643 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:149
> +            m_listenerBreakpoints.set(*eventName, breakpoint.releaseNonNull());

Is it okay that this is no longer an error?

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:159
> +        setAnimationFrameBreakpoint(errorString, WTFMove(breakpoint));

Same question here, though I guess it would be strange to double-set the same event breakpoint. I would still prefer to return an error rather than silently failing.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:238
> +    if (!m_debuggerAgent->breakpointsActive())

Why did the guard move to later?

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:250
> +        breakpoint = domAgent->breakpointForEventListener(*event.currentTarget(), event.type(), registeredEventListener.callback(), registeredEventListener.useCapture());

It's confusing to me that there are two lists of listener breakpoints, in DOMAgent and DOMDebuggerAgent. Can you help me understand why it's designed this way? (I believe that one is a catch-all while the other is for specific listeners in the Node details sidebar panel.)

I think renaming one or the other would help make it clearer as to what this code does. Personally, I'd call the catch-all breakpoints 'm_eventBreakpoints' and the event listeners as written (breakpointForEventListener).

[Blaze Burg]

Comment on attachment 406643 [details]
Patch

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

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:273
> +        JSC::JSLockHolder lock(state);

Oof.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:275
> +        injectedScript.clearEventValue();

Just for easier reference, it would be nice to comment that this functionality is used by `$event`.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:282
> +

Nit: newline

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:311
> +

Idea: maybe we can expose $timer similar to $event while at one of these breakpoints?

> Source/WebCore/inspector/agents/InspectorTimelineAgent.h:105
> +    void breakpointActionProbe(JSC::JSGlobalObject*, JSC::BreakpointActionID, unsigned batchId, unsigned sampleId, JSC::JSValue result) final;

Noice.

> Source/WebInspectorUI/UserInterface/Base/Setting.js:-201
> -    showAllIntervalsBreakpoint: new WI.Setting("show-all-inteverals-breakpoint", false),

lol

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:110
> +            this._allAnimationFramesBreakpoint ??= loadLegacyGlobalEventBreakpoint(WI.EventBreakpoint.Type.AnimationFrame, "show-all-animation-frames-breakpoint", "break-on-all-animation-frames");

Oooo.. k.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:283
> +        if (breakpoint.global) {

Nit: isGlobal()?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:-442
> -            // COMPATIBILITY (iOS 12): DOMDebugger.removeEventBreakpoint did not exist.

I don't see this compat check anywhere else. Did it get lost? Should it exist in more places?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:677
> +        console.assert(breakpoint.removable);

Nice.

> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:79
> +        case WI.domDebuggerManager.allIntervalsBreakpoint:

o_O

[Devin Rousso]

Comment on attachment 406643 [details]
Patch

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

>> Source/JavaScriptCore/inspector/JSGlobalObjectScriptDebugServer.h:-56
>> -    void reportException(JSC::JSGlobalObject*, JSC::Exception*) const final { }
> 
> The comment needs to move to the new declaration. I do think this state of affairs is kind of bad. When there is an exception in the condition or actions, it shows up as an error at the line for the breakpoint. I think it should just be reflected in the actions editor UI itself and mark the breakpoint as "failing" or something (red outline?).
> 
> There are several bugs open such as https://bugs.webkit.org/show_bug.cgi?id=158817 which try to address this, but it's been subpar for a long time.

Oh wow I totally forgot the comment 😱

I'll move it to `Debugger.h`.

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1054
>> +        if (!protocolBreakpoint.matches(scriptURLForBreakpoints))
> 
> This would read better as .matchesURL (it doesn't check line/column).

good point

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:1241
>> +
> 
> Do we expect any more entries to remain in m_protocolBreakpointForProtocolBreakpointID at this point? removeBreakpoint() should have cleared the entries one by one.

yes, as there may be breakpoints set/saved by the frontend from when inspecting a different page that haven't been resolved to any scripts actually loaded

1. inspect <a.com>
2. set breakpoint on <a.com/a.js>
3. navigate to <b.com>
  => protocol breakpoint on <a.com/a.js> still exists even though there are no more debugger breakpoints for it

>> Source/JavaScriptCore/runtime/JSMicrotask.cpp:95
>> +    if (UNLIKELY(globalObject->hasDebugger()))
> 
> It's worrying that this omission wasn't caught by a test. Can you fix that?

So I believe this omission is actually mostly irrelevant, as the only way for this to cause an issue is for the JS microtask that runs to not have pause-able code inside it, which I think only applies to JS microtasks created by WebKit.  Previously, if this is possible, all that would happen is that JS would pause the next time it runs after the WebKit JS microtask finishes.  Now, however, it's a bit more serious because the conditions/actions could execute at the wrong position.

I'll see about writing a test to confirm the new behavior.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:149
>> +            m_listenerBreakpoints.set(*eventName, breakpoint.releaseNonNull());
> 
> Is it okay that this is no longer an error?

I think there's two options here:

1) Allow `setEventBreakpoint` to always work regardless of whether a breakpoint already exists for the given `eventName`.  This means that the frontend can just re-invoke `setEventBreakpoint` to override any previous configuration instead of having to remove the previous configuration and then re-set the new one (i.e. avoids an extra protocol message).
2) Respond with an error if the breakpoint already exists.  This means that the frontend has to remove the previous breakpoint before setting a new one (what `Debugger` already has to do).

Since `Debugger` already does (2) and the previous behavior was (2), we should probably do (2) even though it'll be one extra protocol message (not to mention it'll keep state more understandable).  I'll change it back and adjust the frontend.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:238
>> +    if (!m_debuggerAgent->breakpointsActive())
> 
> Why did the guard move to later?

I personally think we should still set `$event` regardless of whether or not breakpoints pause.  I don't think this makes any difference in practice tho, as `$event` is only available in the command line API, meaning that the only way to use it is when paused inside an event listener.  This also matches `InspectorDOMDebuggerAgent::didHandleEvent` which clears `$event` regardless of whether breakpoints are active.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:250
>> +        breakpoint = domAgent->breakpointForEventListener(*event.currentTarget(), event.type(), registeredEventListener.callback(), registeredEventListener.useCapture());
> 
> It's confusing to me that there are two lists of listener breakpoints, in DOMAgent and DOMDebuggerAgent. Can you help me understand why it's designed this way? (I believe that one is a catch-all while the other is for specific listeners in the Node details sidebar panel.)
> 
> I think renaming one or the other would help make it clearer as to what this code does. Personally, I'd call the catch-all breakpoints 'm_eventBreakpoints' and the event listeners as written (breakpointForEventListener).

This is the difference between global event breakpoints [1] (`DOMDebugger`) and specific listener breakpoints [2] (`DOM`).  I'd rather not use `m_eventBreakpoints` as the concept of an "event" breakpoint also includes `requestAnimationFrame`/`setTimeout`/`setInterval` (not literally but that's how many think of it) which should not be stored in `m_listenerBreakpoints`.  I'd rather rename `InspectorDOMAgent::breakpointForSpecificEventListener`, but I feel like the "Specific" is redundant given the arguments.

If you feel strongly about renaming tho I can change it.

[1]: https://webkit.org/web-inspector/event-breakpoints/#global-event-breakpoints
[2]: https://webkit.org/web-inspector/event-breakpoints/#specific-listener-breakpoints

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:275
>> +        injectedScript.clearEventValue();
> 
> Just for easier reference, it would be nice to comment that this functionality is used by `$event`.

good idea

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:282
>> +
> 
> Nit: newline

i'll actually move this lower so that we don't do the work if one of the earlier branches is taken

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:311
>> +
> 
> Idea: maybe we can expose $timer similar to $event while at one of these breakpoints?

As in `$timerID`?  Unlike `$event` there is no object for `setTimeout`/`setInterval`/`requestAnimationFrame` (the last one has a timestamp tho).  Other than `setInterval`, since you're already executing the callback there's no real use for `$timerID` (it only really exists for canceling).

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.h:105
>> +    void breakpointActionProbe(JSC::JSGlobalObject*, JSC::BreakpointActionID, unsigned batchId, unsigned sampleId, JSC::JSValue result) final;
> 
> Noice.

ikr, so clean

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:283
>> +        if (breakpoint.global) {
> 
> Nit: isGlobal()?

eh, I like having this as a getter (no preference of `global` vs `isGlobal`, although Web Inspector tends to not use `is*` very often in getters)

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:-442
>> -            // COMPATIBILITY (iOS 12): DOMDebugger.removeEventBreakpoint did not exist.
> 
> I don't see this compat check anywhere else. Did it get lost? Should it exist in more places?

this should be handled by the `breakpoint.disabled = true;` at the top of this function, which routes to `WI.DOMDebuggerManager.prototype._handleEventBreakpointDisabledStateChanged` to `WI.DOMDebuggerManager.prototype._updateEventBreakpoint` which calls `DOMDebugger.removeEventBreakpoint`

>> Source/WebInspectorUI/UserInterface/Models/EventBreakpoint.js:79
>> +        case WI.domDebuggerManager.allIntervalsBreakpoint:
> 
> o_O

I love JS `switch` 🤩

[Devin Rousso]

Created attachment 406772 [details]
Patch

[Blaze Burg]

Comment on attachment 406772 [details]
Patch

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

Other than my most recent questions, I think this patch is in good shape. Great work!

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:-42
> -        this.status = WI.ImageUtilities.useSVGSymbol("Images/Breakpoint.svg");

Where did this code to manage the status indicator go?

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:-45
> -        this.tooltipHandledSeparately = true;

Where did this code go?

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:-50
> -    onattach()

I can't find where the code for any of these old-style handlers lives now. What happened to it?

[Devin Rousso]

Comment on attachment 406772 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:-42
>> -        this.status = WI.ImageUtilities.useSVGSymbol("Images/Breakpoint.svg");
> 
> Where did this code to manage the status indicator go?

this should all be on the new common base class `WI.BreakpointTreeElement` (this used to be specifically for JavaScript breakpoints, but given how much all the breakpoints have in common it was generalized and a more specific `WI.JavaScriptBreakpointTreeElement` subclass was created)

>> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:-45
>> -        this.tooltipHandledSeparately = true;
> 
> Where did this code go?

ditto (in the `constructor`)

>> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:-50
>> -    onattach()
> 
> I can't find where the code for any of these old-style handlers lives now. What happened to it?

ditto (specifically `_listenerSet`)

[Devin Rousso]

Comment on attachment 406772 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:77
> +        Introduce the concept of a "special breakpoint", which is essentially a `JSC::Breakpoint`

Thinking about this a bit more, a better name might be "immediate breakpoint" as the word "special" is kinda too vague.  Calling `Debugger::schedulePauseForBreakpoint` (instead of `Debugger::schedulePauseForSpecialBreakpoint`) would set `m_immediateBreakpoint`, which I think is clearer to grok.

I think I'll change this before landing.

[Devin Rousso]

Created attachment 407038 [details]
Patch

I actually thought about this a bit more and I think the term "special" has enough of a clear meaning (not to mention it matches the wording currently used in the docs)

[Devin Rousso]

Created attachment 407086 [details]
Patch

[EWS]

Committed r266074: <https://trac.webkit.org/changeset/266074>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407086 [details].