REGRESSION (r195305): Web Inspector: WebInspector.Object can dispatch constructor-level events multiple times

[Nikita Vasilyev]

In http://trac.webkit.org/changeset/195305 I accidentally undone https://bugs.webkit.org/show_bug.cgi?id=150579,
which re-introduced the bug.

We should bring that object.hasOwnProperty("_listeners") check back.

[Radar WebKit Bug Importer]

<rdar://problem/24253106>

[Nikita Vasilyev]

Created attachment 269340 [details]
Patch

[Joseph Pecoraro]

I think we can write a test for this pretty easily! A test would be great!

[Blaze Burg]

Comment on attachment 269340 [details]
Patch

As Joe said, this needs a test so we don't break it once again.

[Nikita Vasilyev]

Created attachment 269423 [details]
A Test

I wrote a test and I weren't able to reproduce the problem.

I looked closer into Bug 150579 and I realized I don't quite understand
why do we even use this propagation pattern:

    let constructor = this.constructor;
    while (constructor) {
        dispatch(constructor);

        if (!constructor.prototype.__proto__)
            break;

        constructor = constructor.prototype.__proto__.constructor;
    }

https://github.com/WebKit/webkit/blob/707ba5950cfde11635141f386dd497ee1be914e6/Source/WebInspectorUI/UserInterface/Base/Object.js#L170-L178

I commented it out and the only thing that I've noticed stopped working was the styles sidebar.
I'm trying to understand what is happening there.

Does anyone know why do we need this constructor propagation code?

[Timothy Hatcher]

(In reply to comment #5)
> Created attachment 269423 [details]
> A Test
> 
> I wrote a test and I weren't able to reproduce the problem.
> 
> I looked closer into Bug 150579 and I realized I don't quite understand
> why do we even use this propagation pattern:
> 
>     let constructor = this.constructor;
>     while (constructor) {
>         dispatch(constructor);
> 
>         if (!constructor.prototype.__proto__)
>             break;
> 
>         constructor = constructor.prototype.__proto__.constructor;
>     }
> 
> https://github.com/WebKit/webkit/blob/
> 707ba5950cfde11635141f386dd497ee1be914e6/Source/WebInspectorUI/UserInterface/
> Base/Object.js#L170-L178
> 
> I commented it out and the only thing that I've noticed stopped working was
> the styles sidebar.
> I'm trying to understand what is happening there.
> 
> Does anyone know why do we need this constructor propagation code?

This allows you to add a listener to a class or one of its super-classes. So:

Resource.addEventListener("foo", ...);
SourceCode.addEventListener("foo", ...);

If a Resource instance dispatches the "foo" event, it should fire any listeners registered on Resource *and* SourceCode. If a Script (subclass of SourceCode) fired "foo", only the SourceCode listeners should fire.

The hasOwnProperty check prevented the walk up the prototype chain from firing listeners on SourceCode for Script, and then firing them again for SourceCode.

[Timothy Hatcher]

Comment on attachment 269423 [details]
A Test

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

> LayoutTests/inspector/unit-tests/object.html:22
> +            child.addEventListener(eventName, function() {

The listener should be on Parent, not an instance, and not Child to catch the bug.

[Joseph Pecoraro]

> Does anyone know why do we need this constructor propagation code?

A common thing we do is addEventListeners on a constructor `A` to receive all the events that happen for instances of `A`.

There are many cases where a subclass of class `A` will dispatch an event on behalf of the base class.

For example, `DOMTreeContentView is a subclass of `ContentView` but its instances dispatches the `ContentView.Event.SelectionPathComponentsDidChange` event. In fact that event is almost always dispatched by subclasses:

> Views/DOMTreeContentView.js
> 369:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);
> 386:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);

Listeners, like `ContentBrowser` want to handle any `ContentView.Event.SelectionPathComponentsDidChange` for any `ContentView`, so it registers on the `ContentView` constructor:

> Views/ContentBrowser.js
> 71:        WebInspector.ContentView.addEventListener(WebInspector.ContentView.Event.SelectionPathComponentsDidChange, this._contentViewSelectionPathComponentDidChange, this);

So when DOMTreeContentView dispatches the event it will be dispatched at multiple points:

    this._listeners (instance)
    this.constructor._listeners (WebInspector.DOMTreeContentView)
    this.constructor.prototype.__proto__._listeners (WebInspector.ContentView)
    ... (WebInspector.View)
    ... (WebInspectorObject)
    
I suspect removing this could would mean breaking selection path component updates all over the place. Does it?

---

Fun fact. I searched our code for examples of where we register an event on a constructor `WebInspector.Foo` where the event name is not from class Foo (e.g. not `WebInspector.Foo.Event`). I only found one example:

> Controllers/CSSStyleManager.js
> 37:        WebInspector.Resource.addEventListener(WebInspector.SourceCode.Event.ContentDidChange, this._resourceContentDidChange, this);

So, for any Resource instance that dispatches a SourceCode.Event.ContentDidChange event (a superclass event), the cssStyleManager will handle it. That kind of makes sense!

[Nikita Vasilyev]

Created attachment 269531 [details]
Patch

Tim and Joe:
Thanks for the explanation. I hadn't seen event listeners being used on classes directly; it threw me off.

I was able to reproduce the bug. The test now ensures that it's fixed.

[Nikita Vasilyev]

(In reply to comment #8)
> > Does anyone know why do we need this constructor propagation code?
> 
> A common thing we do is addEventListeners on a constructor `A` to receive
> all the events that happen for instances of `A`.
> 
> There are many cases where a subclass of class `A` will dispatch an event on
> behalf of the base class.
> 
> For example, `DOMTreeContentView is a subclass of `ContentView` but its
> instances dispatches the
> `ContentView.Event.SelectionPathComponentsDidChange` event. In fact that
> event is almost always dispatched by subclasses:
> 
> > Views/DOMTreeContentView.js
> > 369:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);
> > 386:        this.dispatchEventToListeners(WebInspector.ContentView.Event.SelectionPathComponentsDidChange);
> 
> Listeners, like `ContentBrowser` want to handle any
> `ContentView.Event.SelectionPathComponentsDidChange` for any `ContentView`,
> so it registers on the `ContentView` constructor:
> 
> > Views/ContentBrowser.js
> > 71:        WebInspector.ContentView.addEventListener(WebInspector.ContentView.Event.SelectionPathComponentsDidChange, this._contentViewSelectionPathComponentDidChange, this);
> 
> So when DOMTreeContentView dispatches the event it will be dispatched at
> multiple points:
> 
>     this._listeners (instance)
>     this.constructor._listeners (WebInspector.DOMTreeContentView)
>     this.constructor.prototype.__proto__._listeners
> (WebInspector.ContentView)
>     ... (WebInspector.View)
>     ... (WebInspectorObject)
>     
> I suspect removing this could would mean breaking selection path component
> updates all over the place. Does it?

Yes, the navigation bar is empty for the Elements tab.
Seems to work well for Resources/Debugger though https://cloudup.com/cBgvs4rBVS3.

[WebKit Commit Bot]

Comment on attachment 269531 [details]
Patch

Clearing flags on attachment: 269531

Committed r195442: <http://trac.webkit.org/changeset/195442>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.