Bug 30337

Summary: Web Inspector: Should be able to delete nodes from the Elements Tree
Product: WebKit Reporter: Brian Weinstein <bweinstein>
Component: Web Inspector (Deprecated)Assignee: Brian Weinstein <bweinstein>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, joepeck, pfeldman, rik, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Fix
timothy: review-, bweinstein: commit-queue-
Addresses Tim's comments
timothy: review+, bweinstein: commit-queue-
Fixes Leak pfeldman: review+, bweinstein: commit-queue-

Description Brian Weinstein 2009-10-13 10:49:41 PDT
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.
Comment 1 Brian Weinstein 2009-10-13 11:00:58 PDT
Created attachment 41113 [details]
Fix
Comment 2 Timothy Hatcher 2009-10-13 11:05:52 PDT
Comment on attachment 41113 [details]
Fix

Feedback given in person.
Comment 3 Brian Weinstein 2009-10-13 11:45:34 PDT
Created attachment 41116 [details]
Addresses Tim's comments
Comment 4 Timothy Hatcher 2009-10-13 11:55:04 PDT
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.
Comment 5 Pavel Feldman 2009-10-13 12:08:47 PDT
> +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.
Comment 6 Brian Weinstein 2009-10-13 12:55:57 PDT
Created attachment 41118 [details]
Fixes Leak
Comment 7 Pavel Feldman 2009-10-13 13:01:55 PDT
Comment on attachment 41118 [details]
Fixes Leak

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

Use removedNodeId === -1 instead.
Comment 8 Brian Weinstein 2009-10-15 14:32:43 PDT
Fix committed in http://trac.webkit.org/changeset/49505, and leak fix commited in http://trac.webkit.org/changeset/49506.