Web Inspector: WebInspector.Object can dispatch constructor-level events multiple times

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
WebInspector.Object can dispatch constructor-level events multiple times. As Object.dispatchEventToListeners walks the prototype chain looking for listeners on each constructor, it should check for listeners that are a direct property of the constructor, rather than a simple 'in' test which will pick up properties inherited down the chain.

* EXAMPLE
Assuming the following class hierarchy:
C -> B -> A -> WebInspector.Object

and an object Foo listening for events on class A:

class Foo() {
   constructor() {
      A.addEventListener(A.Event.SomeEvent, this._handleSomeEvent, this);
   }

   _handleSomeEvent(event) {
      ...
   }
}

If an object of type C fires A.Event.SomeEvent, Foo._handleSomeEvent will be called three times, once for each constructor in the prototype chain. The expectation is that it will be called just once.

* NOTE
This has real performance implications in the Inspector UI, as constructor event handlers often perform layout updates. For example, ContentBrowser calls NavigationBar.udpateLayout (which is extremely expensive) in response to WebInspector.ContentView.Event.SelectionPathComponentsDidChange, which it listens for on the ContentView constructor.

Selecting a DOM node in the Elements tab currently generates *six* calls to NavigationBar.updateLayout (three for each deselect/select in the DOM tree content view). Fixing this issue will reduce the number to just two.

[Radar WebKit Bug Importer]

<rdar://problem/23267238>

[Matt Baker]

Created attachment 264093 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 264093 [details]
[Patch] Proposed Fix

Nice find!

We might be able to keep this code as-is and just remove the loop where we walk the prototype chain and just do: dispatch(this.constructor);

I think that will do the same thing, and just take advantage of JS's inheritance. I'm not sure why I didn't think of that before, unless this wasn't broken until we switched to classes with proper inheritance.

[Matt Baker]

(In reply to comment #3)
> Comment on attachment 264093 [details]
> [Patch] Proposed Fix
> 
> Nice find!
> 
> We might be able to keep this code as-is and just remove the loop where we
> walk the prototype chain and just do: dispatch(this.constructor);

That would be a nice simplification. I'll try it out.

> I think that will do the same thing, and just take advantage of JS's
> inheritance. I'm not sure why I didn't think of that before, unless this
> wasn't broken until we switched to classes with proper inheritance.

Interesting...

[Timothy Hatcher]

Comment on attachment 264093 [details]
[Patch] Proposed Fix

Nevermind! That approach wouldn't work, this is the correct fix.

My idea wont allow proper bubbling to all the classes. It would only catch event listeners registered on the concrete class of the instance, not the superclasses

[WebKit Commit Bot]

Comment on attachment 264093 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 264093

Committed r191615: <http://trac.webkit.org/changeset/191615>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.