Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29410614: Noissue - Make _listeners a Set object (Closed)

Created:
April 12, 2017, 12:17 p.m. by Manish Jethani
Modified:
April 12, 2017, 5:25 p.m.
Visibility:
Public.

Description

Use a Set object instead of an array.

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -7 lines) Patch
M ext/common.js View 1 chunk +4 lines, -7 lines 5 comments Download

Messages

Total messages: 10
Manish Jethani
Patch Set 1 Use a Set object instead of an array.
April 12, 2017, 12:21 p.m. (2017-04-12 12:21:09 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newcode40 ext/common.js:40: let listeners = [...this._listeners]; Is turning the set into ...
April 12, 2017, 12:32 p.m. (2017-04-12 12:32:14 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newcode40 ext/common.js:40: let listeners = [...this._listeners]; On 2017/04/12 12:32:14, Sebastian Noack ...
April 12, 2017, 12:37 p.m. (2017-04-12 12:37:50 UTC) #3
Sebastian Noack
LGTM https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newcode40 ext/common.js:40: let listeners = [...this._listeners]; On 2017/04/12 12:37:50, Manish ...
April 12, 2017, 12:42 p.m. (2017-04-12 12:42:36 UTC) #4
Manish Jethani
Merged. https://hg.adblockplus.org/adblockpluschrome/rev/396ad2a01718
April 12, 2017, 1:38 p.m. (2017-04-12 13:38:53 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newcode40 ext/common.js:40: let listeners = [...this._listeners]; On 2017/04/12 12:42:36, Sebastian Noack ...
April 12, 2017, 2:43 p.m. (2017-04-12 14:43:03 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js File ext/common.js (right): https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newcode40 ext/common.js:40: let listeners = [...this._listeners]; On 2017/04/12 14:43:02, Wladimir Palant ...
April 12, 2017, 2:49 p.m. (2017-04-12 14:49:30 UTC) #7
Manish Jethani
On 2017/04/12 14:43:03, Wladimir Palant wrote: > https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js > File ext/common.js (right): > > https://codereview.adblockplus.org/29410614/diff/29410615/ext/common.js#newcode40 ...
April 12, 2017, 3:24 p.m. (2017-04-12 15:24:48 UTC) #8
Manish Jethani
On 2017/04/12 15:24:48, Manish Jethani wrote: > I just tested this on Node.js, Chrome, and ...
April 12, 2017, 3:42 p.m. (2017-04-12 15:42:49 UTC) #9
Sebastian Noack
April 12, 2017, 5:25 p.m. (2017-04-12 17:25:16 UTC) #10
Message was sent while issue was closed.
On 2017/04/12 15:42:49, Manish Jethani wrote:
> On 2017/04/12 15:24:48, Manish Jethani wrote:
> 
> > I just tested this on Node.js, Chrome, and Firefox, and the array approach
is
> > significantly faster. I can see how this could be optimized, but it isn't.
> Using
> > a new set just adds additional overhead.
> 
> Having said this, maybe we should reconsider why we're making a copy in the
> first place. A set can be modified on the fly while iterating over it. It
means
> it should be safe to remove any listeners whether they have been called or
not.

This is the original issue: https://issues.adblockplus.org/ticket/3427

It seems the problem back then was that a listener that removed itself caused
the next listener to be skipped. This indeed wouldn't be a problem with sets
anymore. But there would still be one scenario in which the behavior differs,
that is if one listener removes another listener that has been registered after
the former listener. When first copying the listeners, that listener would still
be called, otherwise not. I cannot think of any reason why you should have two
listeners for the same event that depend on each other. So fine with me.

Powered by Google App Engine
This is Rietveld