Bug 195146 – Web Inspector: View.removeSubview not removing the element properly when not parented

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

[Joseph Pecoraro]

View.removeSubview not removing the element properly when not parented

I addressed a review comment incompletely... I updated the assertion appropriately but not the code.

[Joseph Pecoraro]

Created attachment 363193 [details]
[PATCH] Proposed Fix

Would only be possible right now debugging Worker Threads in CPU Timeline View.

1. Reload maps.google.com a few times with CPU timeline view

[Matt Baker]

Comment on attachment 363193 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:10
> +        Since the element may not be a direct child, just use Element.prototype.remove.

Nice fix!

I think we should add an assertion around View.js:110, before we try to insert the child view's DOM element:

console.assert(!view.element.parentNode || this._element.contains(view.element.parentNode), "Subview DOM element must be a descendant of the parent view element.");

This can balance the assertion at the top of View.removeSubview.

[Matt Baker]

Comment on attachment 363193 [details]
[PATCH] Proposed Fix

r=me

We should add a test to LayoutTests/inspector/view/basics.html:

suite.addTestCase({
    name: "View.removeSubview.IndirectDescendant",
    test() {
        let parent = new WI.View;
        let middleElement = parent.element.appendChild(document.createElement("div"));

        let child = new WI.View;
        middleElement.appendChild(child.element);

        parent.addSubview(child);
        parent.removeSubview(child);

        InspectorTest.expectFalse(child.element.parentNode, "Removed view should not be in the DOM.");

        return true;
    }
});

[Joseph Pecoraro]

Great suggestions, taking both!

[Joseph Pecoraro]

https://trac.webkit.org/r242241

[Radar WebKit Bug Importer]

<rdar://problem/48494372>