[webkit-reviews] review granted: [Bug 234572] Web Inspector: Assertion Failed removing subview in ContentViewContainer.prototype._disassociateFromContentView : [Attachment 447741] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 234572: Web Inspector: Assertion Failed removing subview in
ContentViewContainer.prototype._disassociateFromContentView
https://bugs.webkit.org/show_bug.cgi?id=234572

Attachment 447741: Patch v1.0

https://bugs.webkit.org/attachment.cgi?id=447741&action=review




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 447741
  --> https://bugs.webkit.org/attachment.cgi?id=447741
Patch v1.0

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

r=me

> Source/WebInspectorUI/UserInterface/Views/ContentViewContainer.js:429
> +	       this.removeSubview(contentView);

So this call was added in r283728 and was originally guarded by
`contentView.constructor.shouldNotRemoveFromDOMWhenHidden()`.  I'm not really
sure if we want this to be something that _always_ happens outside of that
check (especially since almost all callers of this will call `_hideEntry` right
before, which also does `removeSubview`).  Maybe we should restructure this so
that it's all underneath
`contentView.constructor.shouldNotRemoveFromDOMWhenHidden()`?
```
// Hidden/non-visible extension tabs must remain attached to the DOM to avoid
reloading.
if (contentView.constructor.shouldNotRemoveFromDOMWhenHidden()) {
    if (!contentView.visible)
	return;

    if (contentView.attached)
	this.removeSubview(contentView);
}
```