Issue 29338969: Issue 3862 - Add EventEmitter class and implement in the FilterNotifier (Closed)
|
Created:
March 23, 2016, 2:34 p.m. by Sebastian Noack Modified:
March 23, 2016, 5:15 p.m. Reviewers:
Wladimir Palant CC:
kzar, Erik Visibility:
Public. |
DescriptionIssue 3862 - Add EventEmitter class and implement in the FilterNotifier
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixed typo in comment #Patch Set 3 : Use arrow functions #
MessagesTotal messages: 8
Didn't we already add the events script in another review? https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js
On 2016/03/23 15:50:20, kzar wrote: > Didn't we already add the events script in another review? > https://codereview.adblockplus.org/29338534/diff/29338952/lib/events.js We added id to adblockpluschrome, but in order to use it here we have to move it to core.
https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:21: * Registers and emits names events. names => named https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:72: let listener = function() What about arguments? For this to be usable these need to be passed on to resolve(). https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:79: }.bind(this)); Use an arrow function instead of bind(this)? https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:88: emit: function(name) Heh, I can see where you got this from but naming this emit will make using it for Port harder... https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:95: args.push(arguments[i]); Just use Array functions: let args = [].slice.call(arguments, 1); I'd suggest using Array.slice() but Chrome doesn't implement it... https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:99: listener.apply(null, args); Catch errors here so that remaining listeners are still called? https://codereview.adblockplus.org/29338969/diff/29338970/lib/filterNotifier.js File lib/filterNotifier.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/filterNotifier.... lib/filterNotifier.js:33: __proto__: new EventEmitter(), No need creating instances here, EventEmitter.prototype will do.
https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:21: * Registers and emits names events. On 2016/03/23 16:01:07, Wladimir Palant wrote: > names => named Done. https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:72: let listener = function() On 2016/03/23 16:01:07, Wladimir Palant wrote: > What about arguments? For this to be usable these need to be passed on to > resolve(). I thought about passing arguments, but passing in an array would be kinda ugly, and after all as far as I'm aware, this method is only useful for the initialization code in adblockpluschrome where we need a promise that resolves on the initial "load" action, where we don't bother about arguments anyway. So I decided to omit arguments for now, that we can still decide whether to pass in an array or just the first argument, without the risk of breaking anything, once we need it. Do you disagree? https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:79: }.bind(this)); On 2016/03/23 16:01:07, Wladimir Palant wrote: > Use an arrow function instead of bind(this)? I agree that automatically bound arrow functions would be nicer here. However, jsHydra doesn't do the bind-magic when converting arrow functions. Sure, I could still use arrow functions and explicitly bind them, but that will cause them to be bound twice on Firefox. https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:88: emit: function(name) On 2016/03/23 16:01:07, Wladimir Palant wrote: > Heh, I can see where you got this from but naming this emit will make using it > for Port harder... Well, that is just the regular node.js name for that method. As for reusing this class for messaging.Port, I suggest to wrap the event emitter rather than inheriting from it. That is how we did it in adblockpluschrome. https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:95: args.push(arguments[i]); On 2016/03/23 16:01:07, Wladimir Palant wrote: > Just use Array functions: > > let args = [].slice.call(arguments, 1); > > I'd suggest using Array.slice() but Chrome doesn't implement it... Passing the arguments object to a different function causes a deoptimization on V8. https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:99: listener.apply(null, args); On 2016/03/23 16:01:07, Wladimir Palant wrote: > Catch errors here so that remaining listeners are still called? We didn't do that before for FilterStorage, Prefs, etc. Neither does node.js catch errors when calling listeners. Moreover, any try/catch causes the whole function to run in interpreted mode on V8. So rather not? https://codereview.adblockplus.org/29338969/diff/29338970/lib/filterNotifier.js File lib/filterNotifier.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/filterNotifier.... lib/filterNotifier.js:33: __proto__: new EventEmitter(), On 2016/03/23 16:01:07, Wladimir Palant wrote: > No need creating instances here, EventEmitter.prototype will do. Nope, it won't. FilterNotifier isn't a class.
https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:79: }.bind(this)); On 2016/03/23 16:23:43, Sebastian Noack wrote: > However, > jsHydra doesn't do the bind-magic when converting arrow functions. It does: https://hg.adblockplus.org/jshydra/file/tip/scripts/abprewrite.js#l462 https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:95: args.push(arguments[i]); On 2016/03/23 16:23:44, Sebastian Noack wrote: > Passing the arguments object to a different function causes a deoptimization on > V8. And rest parameters aren't supported before Chrome 47 and completely unsupported on Safari of course :-( https://codereview.adblockplus.org/29338969/diff/29338970/lib/filterNotifier.js File lib/filterNotifier.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/filterNotifier.... lib/filterNotifier.js:33: __proto__: new EventEmitter(), On 2016/03/23 16:23:44, Sebastian Noack wrote: > Nope, it won't. FilterNotifier isn't a class. So the issue is apparently that EventEmitter constructor has to run in order to initialize _listeners.
https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js File lib/events.js (right): https://codereview.adblockplus.org/29338969/diff/29338970/lib/events.js#newco... lib/events.js:79: }.bind(this)); On 2016/03/23 16:34:09, Wladimir Palant wrote: > On 2016/03/23 16:23:43, Sebastian Noack wrote: > > However, > > jsHydra doesn't do the bind-magic when converting arrow functions. > > It does: https://hg.adblockplus.org/jshydra/file/tip/scripts/abprewrite.js#l462 Awesome! I didn't realize because it only does if this is used, which is even more brilliant.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
