Bug 30337 - Web Inspector: Should be able to delete nodes from the Elements Tree
Summary: Web Inspector: Should be able to delete nodes from the Elements Tree
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Weinstein
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-13 10:49 PDT by Brian Weinstein
Modified: 2009-10-15 14:32 PDT (History)
5 users (show)

See Also:


Attachments
Fix (6.90 KB, patch)
2009-10-13 11:00 PDT, Brian Weinstein
timothy: review-
bweinstein: commit-queue-
Details | Formatted Diff | Diff
Addresses Tim's comments (6.83 KB, patch)
2009-10-13 11:45 PDT, Brian Weinstein
timothy: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff
Fixes Leak (2.54 KB, patch)
2009-10-13 12:55 PDT, Brian Weinstein
pfeldman: review+
bweinstein: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.