Bug 30337 – Web Inspector: Should be able to delete nodes from the Elements Tree

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Brian Weinstein]

If you are trying to create a reduced test case, or figure out which element is causing layout bugs, it would be nice to be able to delete nodes from the Web Inspector Elements Tree by pressing delete when a node is selected.

[Brian Weinstein]

Created attachment 41113 [details]
Fix

[Timothy Hatcher]

Comment on attachment 41113 [details]
Fix

Feedback given in person.

[Brian Weinstein]

Created attachment 41116 [details]
Addresses Tim's comments

[Timothy Hatcher]

Comment on attachment 41116 [details]
Addresses Tim's comments

> +    if (code != 0)

No need for "!= 0" here.


> +        var self = this;

Move this line closer to the removeNodeCallback function.


> +        function removeNodeCallback(removedNodeId) {

Put the brace on the next line.

[Pavel Feldman]

> +void InspectorBackend::removeNode(long callId, long nodeId)
> +{    
> +    Node* node = nodeForId(nodeId);
> +    if (!node)
> +        return;
> +
> +    Node* parentNode = node->parentNode();
> +    if (!parentNode)
> +        return;
> +
> +    ExceptionCode code;
> +    parentNode->removeChild(node, code);
> +    
> +    if (code != 0)
> +        return;

Returns above prevent you from calling didRemoveNode callback. This results in memory leaks on the frontend side. All callbacks should be actually called so that all functions are released in the frontend. You can add success/result code parameter there if you need code to behave differently depending on the operation outcome.

> -            if (!updateBreadcrumbs && (this.focusedDOMNode === parent || isAncestor(this.focusedDOMNode, parent)))
> +            if (!updateBreadcrumbs && (this.focusedDOMNode === parent || isAncestorNode(this.focusedDOMNode, parent)))

Thanks for fixing it. My git branch for that got lost...

> +        function removeNodeCallback(removedNodeId) {

{ should be on the next line.

[Brian Weinstein]

Created attachment 41118 [details]
Fixes Leak

[Pavel Feldman]

Comment on attachment 41118 [details]
Fixes Leak

> +            if (removedNodeId == -1)
> +                return;

Use removedNodeId === -1 instead.

[Brian Weinstein]

Fix committed in http://trac.webkit.org/changeset/49505, and leak fix commited in http://trac.webkit.org/changeset/49506.