Bug 195146 - Web Inspector: View.removeSubview not removing the element properly when not parented
Summary: Web Inspector: View.removeSubview not removing the element properly when not ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-27 22:57 PST by Joseph Pecoraro
Modified: 2019-02-28 16:49 PST (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (1.27 KB, patch)
2019-02-27 22:58 PST, Joseph Pecoraro
mattbaker: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-02-27 22:57:37 PST
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.
Comment 1 Joseph Pecoraro 2019-02-27 22:58:43 PST
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
Comment 2 Matt Baker 2019-02-28 00:30:08 PST
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.
Comment 3 Matt Baker 2019-02-28 00:35:11 PST
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;
    }
});
Comment 4 Joseph Pecoraro 2019-02-28 16:20:11 PST
Great suggestions, taking both!
Comment 5 Joseph Pecoraro 2019-02-28 16:48:39 PST
https://trac.webkit.org/r242241
Comment 6 Radar WebKit Bug Importer 2019-02-28 16:49:33 PST
<rdar://problem/48494372>