Summary: | Web Inspector: remove event listeners from SpreadsheetStyleProperty and SpreadsheetCSSStyleDeclarationEditor on detachment | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||
Status: | ASSIGNED --- | ||||||||||
Severity: | Normal | CC: | hi, inspector-bugzilla-changes, mattbaker, timothy | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2019-03-05 15:31:58 PST
Created attachment 363696 [details]
Patch
Created attachment 363699 [details]
Patch
Created attachment 363702 [details]
Patch
Comment on attachment 363702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363702&action=review I'm not super familiar with `WI.View.prototype.detached` (and `WI.View.prototype.attached`), so I'm not sure when exactly `detached` (and `attached`) get called. If it only get's called when we call `removeSubview`, then I don't think there are any issues (although I still think we should change them). If it gets called whenever any parent gets removed (e.g. switching tabs), then we have a problem and need to make these changes. Based on what I can tell, I think it's the latter. @mattbaker, is this correct? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:128 > + if (this._style) > + this._style.removeEventListener(null, null, this); Should we be re-adding these when we call `attached`? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:130 > + super.detached(); Nice! > Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:128 > + this._property.removeEventListener(null, null, this); Ditto (>SpreadsheetCSSStyleDeclarationEditor.js:127). (In reply to Devin Rousso from comment #4) > Comment on attachment 363702 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=363702&action=review > > I'm not super familiar with `WI.View.prototype.detached` (and > `WI.View.prototype.attached`), so I'm not sure when exactly `detached` (and > `attached`) get called. If it only get's called when we call > `removeSubview`, then I don't think there are any issues (although I still > think we should change them). If it gets called whenever any parent gets > removed (e.g. switching tabs), then we have a problem and need to make these > changes. Based on what I can tell, I think it's the latter. > > @mattbaker, is this correct? These methods are called when the View is parented in the DOM: let root = WI.View.rootView(); let parent = new WI.View; let child = new WI.View; parent.addSubview(child) // child isn't attached, since parent isn't attached root.addSubview(parent) // attached() called for parent, then child parent.removeSubview(child) // detached() called for child Comment on attachment 363702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363702&action=review > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125 > propertyView.detached(); Since propertyViews are Objects, not Views, this method should be called `closed()`, instead of `detached()`, so as not to cause confusion. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:128 >> + this._style.removeEventListener(null, null, this); > > Should we be re-adding these when we call `attached`? Unless the View is kept alive after being removed (it's reused or something), it shouldn't really be necessary to override View.prototype.detached. Is it needed because this._style is an external object that would continue to hold references to the View otherwise? If the view is being re-used, we would want to re-add the listeners as Devin suggested, but I suspect we're doing this for the reason mentioned above. Comment on attachment 363702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363702&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125 >> propertyView.detached(); > > Since propertyViews are Objects, not Views, this method should be called `closed()`, instead of `detached()`, so as not to cause confusion. How about removed()? closed() makes me think of a modal or sidebar being closed. >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:128 >>> + this._style.removeEventListener(null, null, this); >> >> Should we be re-adding these when we call `attached`? > > Unless the View is kept alive after being removed (it's reused or something), it shouldn't really be necessary to override View.prototype.detached. Is it needed because this._style is an external object that would continue to hold references to the View otherwise? > > If the view is being re-used, we would want to re-add the listeners as Devin suggested, but I suspect we're doing this for the reason mentioned above. "Is it needed because this._style is an external object that would continue to hold references to the View otherwise?" — yes, exactly. The view is being re-added when switching tabs. Good catch! Comment on attachment 363702 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363702&action=review >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125 >>> propertyView.detached(); >> >> Since propertyViews are Objects, not Views, this method should be called `closed()`, instead of `detached()`, so as not to cause confusion. > > How about removed()? closed() makes me think of a modal or sidebar being closed. Why is property view called view if it ins't a View? Close is usually an end-of-life type of action in WebKit. I like detached or removed. |