Bug 195343

Summary: Web Inspector: remove event listeners from SpreadsheetStyleProperty and SpreadsheetCSSStyleDeclarationEditor on detachment
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: 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 Flags
Patch
none
Patch
none
Patch timothy: review-

Description Nikita Vasilyev 2019-03-05 15:31:58 PST
This should be cleaned up:

        property.addEventListener(WI.CSSProperty.Event.OverriddenStatusChanged, this.updateStatus, this);
        property.addEventListener(WI.CSSProperty.Event.Changed, this.updateStatus, this);
Comment 1 Nikita Vasilyev 2019-03-05 15:34:07 PST Comment hidden (obsolete)
Comment 2 Nikita Vasilyev 2019-03-05 15:37:18 PST Comment hidden (obsolete)
Comment 3 Nikita Vasilyev 2019-03-05 15:45:07 PST
Created attachment 363702 [details]
Patch
Comment 4 Devin Rousso 2019-03-05 18:05:29 PST
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).
Comment 5 Matt Baker 2019-03-05 20:08:08 PST
(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 6 Matt Baker 2019-03-05 20:19:45 PST
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 7 Nikita Vasilyev 2019-03-06 15:26:13 PST
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 8 Timothy Hatcher 2019-03-20 09:21:07 PDT
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.