Bug 203349 - Web Inspector: DOMDebugger: Node Removed breakpoints should fire whenever the node is removed from the main DOM tree, not just when it's removed from it's parent
Summary: Web Inspector: DOMDebugger: Node Removed breakpoints should fire whenever the...
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
Depends on:
Blocks:
 
Reported: 2019-10-23 16:34 PDT by Devin Rousso
Modified: 2019-10-31 13:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (20.17 KB, patch)
2019-10-23 18:01 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.77 KB, patch)
2019-10-30 22:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (20.77 KB, patch)
2019-10-30 22:22 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 2019-10-23 16:34:44 PDT
If I have a DOM tree

    <a>
        <b>
            <c>

where <c> has a Node Removed DOM breakpoint, we currently only trigger that breakpoint of <c> is removed from <b>, but not if <b> is removed from <a> or if <a> is removed from its parent (and so on, all the way up to the root).  We should treat the Node Removed DOM breakpoint as a "trigger when this node is about to be removed from the main DOM tree" instead of "trigger when this node is about to be removed from its parent".
Comment 1 Devin Rousso 2019-10-23 18:01:25 PDT
Created attachment 381760 [details]
Patch
Comment 2 Matt Baker 2019-10-24 12:37:53 PDT
Comment on attachment 381760 [details]
Patch

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

Nice! Great test clean up too. A few minor comments.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

A short comment mentioning the change to `pauseData` would be nice.

> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:197
> +    {

Why the nested scope? Nested scopes are usually to control the lifetime of an RAII object.

> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:255
> +        name: "NodeRemovedDirect",

How about "NodeRemovedSelf"? It pairs nicely with "NodeRemovedAncestor", whereas "Direct" makes me think of something as opposed to "indirect", which also sort of makes sense but isn't how we usually refer to DOM relationships.
Comment 3 Devin Rousso 2019-10-25 16:09:02 PDT
Comment on attachment 381760 [details]
Patch

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

>> Source/WebCore/ChangeLog:6
>> +        Reviewed by NOBODY (OOPS!).
> 
> A short comment mentioning the change to `pauseData` would be nice.

Can do :)

>> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:197
>> +    {
> 
> Why the nested scope? Nested scopes are usually to control the lifetime of an RAII object.

I did this to make it clearer what `rootBit`, `derivedBit`, and `matchBit` are used for.  I often use scopes for more than just RAII, as it's a nice way of segmenting code.

>> LayoutTests/inspector/dom-debugger/dom-breakpoints.html:255
>> +        name: "NodeRemovedDirect",
> 
> How about "NodeRemovedSelf"? It pairs nicely with "NodeRemovedAncestor", whereas "Direct" makes me think of something as opposed to "indirect", which also sort of makes sense but isn't how we usually refer to DOM relationships.

Ooo I like it!
Comment 4 Matt Baker 2019-10-25 16:24:59 PDT
Comment on attachment 381760 [details]
Patch

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

>>> Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:197
>>> +    {
>> 
>> Why the nested scope? Nested scopes are usually to control the lifetime of an RAII object.
> 
> I did this to make it clearer what `rootBit`, `derivedBit`, and `matchBit` are used for.  I often use scopes for more than just RAII, as it's a nice way of segmenting code.

I think this goes against the grain of typical WebKit C++ style. I'd rather we didn't use nested scopes like this.
Comment 5 Devin Rousso 2019-10-30 22:19:34 PDT
Created attachment 382426 [details]
Patch
Comment 6 Devin Rousso 2019-10-30 22:22:27 PDT
Created attachment 382427 [details]
Patch

This is what I get for trying to manually edit a diff -.-
Comment 7 Matt Baker 2019-10-31 12:24:49 PDT
Comment on attachment 382427 [details]
Patch

r=me, nice!
Comment 8 WebKit Commit Bot 2019-10-31 13:20:51 PDT
Comment on attachment 382427 [details]
Patch

Clearing flags on attachment: 382427

Committed r251871: <https://trac.webkit.org/changeset/251871>
Comment 9 WebKit Commit Bot 2019-10-31 13:20:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2019-10-31 13:21:16 PDT
<rdar://problem/56792748>