[webkit-reviews] review granted: [Bug 196956] Web Inspector: use weak collections for holding event listeners : [Attachment 411030] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 196956: Web Inspector: use weak collections for holding event listeners
https://bugs.webkit.org/show_bug.cgi?id=196956

Attachment 411030: Patch

https://bugs.webkit.org/attachment.cgi?id=411030&action=review




--- Comment #34 from Brian Burg <bburg at apple.com> ---
Comment on attachment 411030
  --> https://bugs.webkit.org/attachment.cgi?id=411030
Patch

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

r=me, great work!

> Source/WebInspectorUI/ChangeLog:265
> +	   Remove the `closed` method that only `removeEventListener` as they
are no longer needed

Nit: grammaro

> Source/WebInspectorUI/UserInterface/Base/Object.js:60
> +	   let wrappedCallback = (...args) => {

Nice to get rid of unneeded `arguments`!

> Source/WebInspectorUI/UserInterface/Base/Object.js:124
>	       // Make a copy with slice so mutations during the loop doesn't
affect us.

Nit: unnecessary comment.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:566
> +	   let promises = [this.awaitEvent(WI.DebuggerManager.Event.Paused,
this)];

Nice!

> Source/WebInspectorUI/UserInterface/Debug/Bootstrap.js:38
> +}, WI.settings.engineeringPauseForInternalScripts);

LOL, oops

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1037
> +		  
this.singleFireEventListener(WI.Resource.Event.LoadingDidFinish, resolve,
this);

Nice.

> Source/WebInspectorUI/UserInterface/Views/AuditTreeElement.js:100
> +	   if (this.representedObject instanceof WI.AuditTestBase) {

Cranky logic like this is why I wrote EventListenerSet in the first place.

> Source/WebInspectorUI/UserInterface/Views/DataGridNode.js:493
> +	       this.dispatchEventToListeners(DataGridNode.Event.Populate);

O_O

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-108
> -	  
WI.HeapSnapshotWorkerProxy.singleton().addEventListener("HeapSnapshot.Collectio
nEvent", this._heapSnapshotCollectionEvent, this);

o_O

> Source/WebInspectorUI/UserInterface/Views/HeapAllocationsTimelineView.js:-264
> -	  
WI.HeapSnapshotWorkerProxy.singleton().removeEventListener("HeapSnapshot.Collec
tionEvent", this._heapSnapshotCollectionEvent, this);

It seems like we lost this removeEventListener.