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

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

[Devin Rousso]

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

[Devin Rousso]

Created attachment 381760 [details]
Patch

[Matt Baker]

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.

[Devin Rousso]

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!

[Matt Baker]

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.

[Devin Rousso]

Created attachment 382426 [details]
Patch

[Devin Rousso]

Created attachment 382427 [details]
Patch

This is what I get for trying to manually edit a diff -.-

[Matt Baker]

Comment on attachment 382427 [details]
Patch

r=me, nice!

[WebKit Commit Bot]

Comment on attachment 382427 [details]
Patch

Clearing flags on attachment: 382427

Committed r251871: <https://trac.webkit.org/changeset/251871>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56792748>