Summary: | Web Inspector: View.removeSubview not removing the element properly when not parented | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||
Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | inspector-bugzilla-changes, joepeck, mattbaker, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Joseph Pecoraro
2019-02-27 22:57:37 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 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 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;
}
});
Great suggestions, taking both! |