195343 2019-03-05 15:31:58 -0800 Web Inspector: remove event listeners from SpreadsheetStyleProperty and SpreadsheetCSSStyleDeclarationEditor on detachment 2019-03-20 09:21:33 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All ASSIGNED P2 Normal --- 1 nvasilyev nvasilyev hi inspector-bugzilla-changes mattbaker timothy oldest_to_newest 1512867 0 nvasilyev 2019-03-05 15:31:58 -0800 This should be cleaned up: property.addEventListener(WI.CSSProperty.Event.OverriddenStatusChanged, this.updateStatus, this); property.addEventListener(WI.CSSProperty.Event.Changed, this.updateStatus, this); 1512869 1 363696 nvasilyev 2019-03-05 15:34:07 -0800 Created attachment 363696 Patch 1512878 2 363699 nvasilyev 2019-03-05 15:37:18 -0800 Created attachment 363699 Patch 1512886 3 363702 nvasilyev 2019-03-05 15:45:07 -0800 Created attachment 363702 Patch 1512936 4 363702 hi 2019-03-05 18:05:29 -0800 Comment on attachment 363702 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). 1512974 5 mattbaker 2019-03-05 20:08:08 -0800 (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 1512980 6 363702 mattbaker 2019-03-05 20:19:45 -0800 Comment on attachment 363702 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. 1513231 7 363702 nvasilyev 2019-03-06 15:26:13 -0800 Comment on attachment 363702 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! 1518918 8 363702 timothy 2019-03-20 09:21:07 -0700 Comment on attachment 363702 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. 363696 2019-03-05 15:34:07 -0800 2019-03-05 15:37:18 -0800 Patch patch.txt text/plain 1348 nvasilyev ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDU1ZWY4YWMzZTFiLi41ZGMzMzYzMDRkNSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTktMDMtMDUgIE5pa2l0 YSBWYXNpbHlldiAgPG52YXNpbHlldkBhcHBsZS5jb20+CisKKyAgICAgICAgV2ViIEluc3BlY3Rv cjogcmVtb3ZlIGV2ZW50IGxpc3RlbmVycyBmcm9tIFNwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eSBv biBkZXRhY2htZW50CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xOTUzNDMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIFVzZXJJbnRlcmZhY2UvVmlld3MvU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5LmpzOgor ICAgICAgICAoV0kuU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5LnByb3RvdHlwZS5kZXRhY2hlZCk6 CisKIDIwMTktMDMtMDUgIERldmluIFJvdXNzbyAgPGRyb3Vzc29AYXBwbGUuY29tPgogCiAgICAg ICAgIFdlYiBJbnNwZWN0b3I6IENhbnZhczogcmVtb3ZlIGV2ZW50IGxpc3RlbmVycyBvbmNlIGEg cmVjb3JkaW5nIGlzIHJlYWR5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNl ckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHkuanMgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHku anMKaW5kZXggNGQ4ZDI1NDQ0NzUuLmVmYzQ0YWE0N2IwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHku anMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU3ByZWFk c2hlZXRTdHlsZVByb3BlcnR5LmpzCkBAIC0xMjQsNiArMTI0LDEwIEBAIFdJLlNwcmVhZHNoZWV0 U3R5bGVQcm9wZXJ0eSA9IGNsYXNzIFNwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eSBleHRlbmRzIFdJ Lk9iamVjdAogCiAgICAgICAgIGlmICh0aGlzLl92YWx1ZVRleHRGaWVsZCkKICAgICAgICAgICAg IHRoaXMuX3ZhbHVlVGV4dEZpZWxkLmRldGFjaGVkKCk7CisKKyAgICAgICAgdGhpcy5fcHJvcGVy dHkucmVtb3ZlRXZlbnRMaXN0ZW5lcihudWxsLCBudWxsLCB0aGlzKTsKKworICAgICAgICBzdXBl ci5kZXRhY2hlZCgpOwogICAgIH0KIAogICAgIGhpZGRlbigpCg== 363699 2019-03-05 15:37:18 -0800 2019-03-05 15:45:07 -0800 Patch patch.txt text/plain 1318 nvasilyev ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDU1ZWY4YWMzZTFiLi41ZGMzMzYzMDRkNSAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMgQEAKKzIwMTktMDMtMDUgIE5pa2l0 YSBWYXNpbHlldiAgPG52YXNpbHlldkBhcHBsZS5jb20+CisKKyAgICAgICAgV2ViIEluc3BlY3Rv cjogcmVtb3ZlIGV2ZW50IGxpc3RlbmVycyBmcm9tIFNwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eSBv biBkZXRhY2htZW50CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xOTUzNDMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIFVzZXJJbnRlcmZhY2UvVmlld3MvU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5LmpzOgor ICAgICAgICAoV0kuU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5LnByb3RvdHlwZS5kZXRhY2hlZCk6 CisKIDIwMTktMDMtMDUgIERldmluIFJvdXNzbyAgPGRyb3Vzc29AYXBwbGUuY29tPgogCiAgICAg ICAgIFdlYiBJbnNwZWN0b3I6IENhbnZhczogcmVtb3ZlIGV2ZW50IGxpc3RlbmVycyBvbmNlIGEg cmVjb3JkaW5nIGlzIHJlYWR5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNl ckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHkuanMgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHku anMKaW5kZXggNGQ4ZDI1NDQ0NzUuLjZkNGViODk3MmNiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHku anMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU3ByZWFk c2hlZXRTdHlsZVByb3BlcnR5LmpzCkBAIC0xMjQsNiArMTI0LDggQEAgV0kuU3ByZWFkc2hlZXRT dHlsZVByb3BlcnR5ID0gY2xhc3MgU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5IGV4dGVuZHMgV0ku T2JqZWN0CiAKICAgICAgICAgaWYgKHRoaXMuX3ZhbHVlVGV4dEZpZWxkKQogICAgICAgICAgICAg dGhpcy5fdmFsdWVUZXh0RmllbGQuZGV0YWNoZWQoKTsKKworICAgICAgICB0aGlzLl9wcm9wZXJ0 eS5yZW1vdmVFdmVudExpc3RlbmVyKG51bGwsIG51bGwsIHRoaXMpOwogICAgIH0KIAogICAgIGhp ZGRlbigpCg== 363702 2019-03-05 15:45:07 -0800 2019-03-20 09:21:33 -0700 Patch patch.txt text/plain 2236 nvasilyev ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDU1ZWY4YWMzZTFiLi5jZDBjZjExMDNiMCAxMDA2 NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTktMDMtMDUgIE5pa2l0 YSBWYXNpbHlldiAgPG52YXNpbHlldkBhcHBsZS5jb20+CisKKyAgICAgICAgV2ViIEluc3BlY3Rv cjogcmVtb3ZlIGV2ZW50IGxpc3RlbmVycyBmcm9tIFNwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eSBh bmQgU3ByZWFkc2hlZXRDU1NTdHlsZURlY2xhcmF0aW9uRWRpdG9yIG9uIGRldGFjaG1lbnQKKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE5NTM0MworCisg ICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogVXNlckludGVy ZmFjZS9WaWV3cy9TcHJlYWRzaGVldENTU1N0eWxlRGVjbGFyYXRpb25FZGl0b3IuanM6CisgICAg ICAgIChXSS5TcHJlYWRzaGVldENTU1N0eWxlRGVjbGFyYXRpb25FZGl0b3IucHJvdG90eXBlLmRl dGFjaGVkKToKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1NwcmVhZHNoZWV0U3R5bGVQ cm9wZXJ0eS5qczoKKyAgICAgICAgKFdJLlNwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eS5wcm90b3R5 cGUuZGV0YWNoZWQpOgorCiAyMDE5LTAzLTA1ICBEZXZpbiBSb3Vzc28gIDxkcm91c3NvQGFwcGxl LmNvbT4KIAogICAgICAgICBXZWIgSW5zcGVjdG9yOiBDYW52YXM6IHJlbW92ZSBldmVudCBsaXN0 ZW5lcnMgb25jZSBhIHJlY29yZGluZyBpcyByZWFkeQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYklu c3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU3ByZWFkc2hlZXRDU1NTdHlsZURlY2xhcmF0 aW9uRWRpdG9yLmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3Mv U3ByZWFkc2hlZXRDU1NTdHlsZURlY2xhcmF0aW9uRWRpdG9yLmpzCmluZGV4IDA0NDAxYjBiMWE2 Li5hOWNiOGUyMzA5MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRl cmZhY2UvVmlld3MvU3ByZWFkc2hlZXRDU1NTdHlsZURlY2xhcmF0aW9uRWRpdG9yLmpzCisrKyBi L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1NwcmVhZHNoZWV0Q1NT U3R5bGVEZWNsYXJhdGlvbkVkaXRvci5qcwpAQCAtMTIzLDYgKzEyMywxMSBAQCBXSS5TcHJlYWRz aGVldENTU1N0eWxlRGVjbGFyYXRpb25FZGl0b3IgPSBjbGFzcyBTcHJlYWRzaGVldENTU1N0eWxl RGVjbGFyYXRpb25FZAogCiAgICAgICAgIGZvciAobGV0IHByb3BlcnR5VmlldyBvZiB0aGlzLl9w cm9wZXJ0eVZpZXdzKQogICAgICAgICAgICAgcHJvcGVydHlWaWV3LmRldGFjaGVkKCk7CisKKyAg ICAgICAgaWYgKHRoaXMuX3N0eWxlKQorICAgICAgICAgICAgdGhpcy5fc3R5bGUucmVtb3ZlRXZl bnRMaXN0ZW5lcihudWxsLCBudWxsLCB0aGlzKTsKKworICAgICAgICBzdXBlci5kZXRhY2hlZCgp OwogICAgIH0KIAogICAgIGhpZGRlbigpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHkuanMgYi9Tb3Vy Y2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJv cGVydHkuanMKaW5kZXggNGQ4ZDI1NDQ0NzUuLjZkNGViODk3MmNiIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJv cGVydHkuanMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3Mv U3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5LmpzCkBAIC0xMjQsNiArMTI0LDggQEAgV0kuU3ByZWFk c2hlZXRTdHlsZVByb3BlcnR5ID0gY2xhc3MgU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5IGV4dGVu ZHMgV0kuT2JqZWN0CiAKICAgICAgICAgaWYgKHRoaXMuX3ZhbHVlVGV4dEZpZWxkKQogICAgICAg ICAgICAgdGhpcy5fdmFsdWVUZXh0RmllbGQuZGV0YWNoZWQoKTsKKworICAgICAgICB0aGlzLl9w cm9wZXJ0eS5yZW1vdmVFdmVudExpc3RlbmVyKG51bGwsIG51bGwsIHRoaXMpOwogICAgIH0KIAog ICAgIGhpZGRlbigpCg==