Bug 195343 – Web Inspector: remove event listeners from SpreadsheetStyleProperty and SpreadsheetCSSStyleDeclarationEditor on detachment

• Status: ASSIGNED
• Component: Web Inspector

[Nikita Vasilyev]

This should be cleaned up:

        property.addEventListener(WI.CSSProperty.Event.OverriddenStatusChanged, this.updateStatus, this);
        property.addEventListener(WI.CSSProperty.Event.Changed, this.updateStatus, this);

[Nikita Vasilyev]

Created attachment 363696 [details]
Patch

[Nikita Vasilyev]

Created attachment 363699 [details]
Patch

[Nikita Vasilyev]

Created attachment 363702 [details]
Patch

[Devin Rousso]

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).

[Matt Baker]

(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

[Matt Baker]

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.

[Nikita Vasilyev]

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!

[Timothy Hatcher]

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.