Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
245923002:
Bring events to JavaScript components (Closed)
|
Created:
6 years, 8 months ago by kelvinp Modified:
6 years, 8 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionA simple implementation to augment JavaScript objects with events
For example, to create an alarm event for SmokeDetector
function SmokeDetector() {
this.defineEvents(['alarm']);
};
base.augment(SmokeDetector, base.Events);
To fire an event
SmokeDetector.prototype.onCarbonMonoxideDetected = function () {
var param = {} // optional parameters
this.raiseEvent('alarm', param);
}
To listen to an event
var smokeDetector = new SmokeDetector();
smokeDetector.addEventListener('alarm',listenerObj.someCallback,listenerObj);
The code lives in (base.js), a module that contains JavaScript utility components and methods for the web app.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266164
Patch Set 1 : Add base.js and Event Listeners #
Total comments: 80
Patch Set 2 : Address feedbacks #
Total comments: 20
Patch Set 3 : Address feedbacks - format nit and remove this #Patch Set 4 : Simplify event handling #
Total comments: 18
Patch Set 5 : Last round of CL #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. lose (c). 2014 https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:15: var Debug = {}; should this be lower-case 'debug'? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:64: base.mix = function(dest, src) { call this copyObjectProperties()? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:74: * Copies properties from the src to dest.prototype I think "mixin" is the standard terms for things like this. http://en.wikipedia.org/wiki/Mixin . So maybe say something "Adds a mixin to a class"? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:79: base.augment = function(dest, src) { call it base.addClassMixin()? and also rename the argument? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:83: base.emptyFn = function () {}; either emptyFunction() or doNothing() (I would prefer latter because it's a verb and is consistent with base::DoNothing in base/base_helpers.h). https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:86: * A simple implementation to augment JavaScript objects with events "Mixin for classes with events." nit: comments should normally end with . https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:110: * Define the events this event source Comments should end with '.' https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:116: var ev = this._ev = {}; s/ev/event/ https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:129: * Define the events this event source Update this comment https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:132: * @param {*} thisObj Why do we need this argument? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:141: * Define the events this event source update the comment https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:150: var listener; does this need to be defined outside of the for loop? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:191: for (i = listeners.length; i--;) { The following would me more readable: for (var i = listeners.length - 1; i >= 0; i--) 1) you need "- 1" 2) I think you need "var" when defining i here. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:192: if (!listeners[i]) { Why do you need this check? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:206: chk: function (obj, type) { Nt rdbl nm. (not readable name) https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:207: Debug.assert(Boolean(obj._ev), 'The object doesn\'t support events'); nit: double space https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:214: Debug.assert = base.emptyFn; Why not have asserts always on? IMO it's better to have them always on, they just shouln't trigger alert() by default (and just log to the console)
https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:15: var Debug = {}; Is there a reason for the case difference between these two? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:18: * Asserts that expr is true else print the msg s/Asserts/Assert/ https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:22: Debug.assert = function (expr, opt_msg) { Nit: No space after "function" (here and elsewhere). https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:31: * @suppress {checkTypes} Is it possible to make this function type-safe? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:40: var callstack = error.stack Why can't this be in the catch block? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:53: if (Boolean(obj)) { What does the cast to Boolean buy us in this case? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:76: * @param {*} src Why not Object for these two parameters? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:77: * @suppress {checkTypes} Is it possible to make this function type-safe? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:106: base.Events = function () {}; s/Events/EventSource/? When I read Events, I assumed this was a namespace--I think EventSource makes it clearer that it's a base/mix-in class. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:110: * Define the events this event source Incomplete comment. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:121: ev[type] = { The state you're tracking for each event is somewhat subtle, so I don't think it should be buried inside a method. Promote the EventEntry class from the proto to be a class, and maybe give it add/remove/raise methods. Alternatively, I think you might be able to simplify the implementation to store just a list of callbacks if you take a copy of that list before iterating over it. Which approach is better depends on how many callbacks you expect there to be. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:129: * Define the events this event source Copy/paste error? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:132: * @param {*} thisObj I think it would be better to require callers to pass in a bound function rather than an explicit |this| pointer. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:141: * Define the events this event source Copy/paste error. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:166: return; In general, I prefer to use break to exit a loop, even if there's no code following it. It makes the function more robust to future code changes https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:182: for (var i = 0; i < listeners.length; i++) { Can you use forEach here? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:207: Debug.assert(Boolean(obj._ev), 'The object doesn\'t support events'); For strings with embedded apostrophes, you can use double-quotes. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/js_proto... File remoting/webapp/js_proto/base_proto.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/js_proto... remoting/webapp/js_proto/base_proto.js:7: // JSCompiler to verify the type-correctness of our code. This should not be necessary. We have proto files for types defined externally to the web-app, but this is code we're fully in control of. In particular, you can get rid of the @this directive in the implementation file and just rename it to EventSource to get the benefits of most of these directives.
https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:50: * @param {Disposable} obj Disposable isn't base JS. Are you assuming the presence of Closure (or some equivalent framework)? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:102: * smokeDetector.addEventListener('alarm',listenerObj.someCallback,listenerObj); This mechanism seems a bit dangerous from a lifetime management standpoint. Javascript doesn't have weakrefs, so the objects referenced will stay alive until they are manually removed. JS also has no way to enforce that unregister will be called when an object is unrefrenced (e.g. if the listener's owner just assigns a new value to the reference, and so on transitively to the owner's owner), so we run the risk of calling an event on a very stale object that is supposed to be gone. This would be fine in a one-off class, with a very definite lifetime which we can manually inspect to ensure the events are unregistered, but feels a bit fragile to declare a general-purpose base-class.
https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/04/22 00:43:21, Sergey Ulanov wrote: > lose (c). > 2014 Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:15: var Debug = {}; On 2014/04/22 00:43:21, Sergey Ulanov wrote: > should this be lower-case 'debug'? debug(fn) is an existing function implemented by chrome that allows to set a breakpoint on the first line of a function. I don't think I should overwrite it. Renaming it to debugUtil https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:15: var Debug = {}; On 2014/04/22 01:23:21, Jamie wrote: > Is there a reason for the case difference between these two? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:18: * Asserts that expr is true else print the msg On 2014/04/22 01:23:21, Jamie wrote: > s/Asserts/Assert/ Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:22: Debug.assert = function (expr, opt_msg) { On 2014/04/22 01:23:21, Jamie wrote: > Nit: No space after "function" (here and elsewhere). Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:31: * @suppress {checkTypes} On 2014/04/22 01:23:21, Jamie wrote: > Is it possible to make this function type-safe? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:40: var callstack = error.stack On 2014/04/22 01:23:21, Jamie wrote: > Why can't this be in the catch block? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:50: * @param {Disposable} obj On 2014/04/22 02:25:35, rmsousa wrote: > Disposable isn't base JS. Are you assuming the presence of Closure (or some > equivalent framework)? No, I am not assuming the presence of any framework. In fact, we try to avoid bringing it unnecessary framework wherever possible. I can see where the confusion comes from. Disposable is an interface that has the dispose method. It is defined in base_proto.js. Let me move it to base.js to make it less confusing https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:53: if (Boolean(obj)) { On 2014/04/22 01:23:21, Jamie wrote: > What does the cast to Boolean buy us in this case? Trying to pre-emptively make Jscompile happy. I guess jscompile is more content that I thought. removing it https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:64: base.mix = function(dest, src) { On 2014/04/22 00:43:21, Sergey Ulanov wrote: > call this copyObjectProperties()? I will keep it called mix. I think mix is shorter name and mixin is a standard term in JavaScript https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:74: * Copies properties from the src to dest.prototype On 2014/04/22 00:43:21, Sergey Ulanov wrote: > I think "mixin" is the standard terms for things like this. > http://en.wikipedia.org/wiki/Mixin . So maybe say something "Adds a mixin to a > class"? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:76: * @param {*} src On 2014/04/22 01:23:21, Jamie wrote: > Why not Object for these two parameters? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:77: * @suppress {checkTypes} On 2014/04/22 01:23:21, Jamie wrote: > Is it possible to make this function type-safe? Unfortunately, the access of src.prototype and dest.prototype really throws JSCop off. Since this function is a one liner, I don't think we will lose too much safety by turning type checking off here. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:79: base.augment = function(dest, src) { On 2014/04/22 00:43:21, Sergey Ulanov wrote: > call it base.addClassMixin()? and also rename the argument? Augment and extend are the standard terms. I think I will change to to extend, as that name is more well used in other libraries like google closure or jquery. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:83: base.emptyFn = function () {}; On 2014/04/22 00:43:21, Sergey Ulanov wrote: > either emptyFunction() or doNothing() > (I would prefer latter because it's a verb and is consistent with > base::DoNothing in base/base_helpers.h). Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:86: * A simple implementation to augment JavaScript objects with events On 2014/04/22 00:43:21, Sergey Ulanov wrote: > "Mixin for classes with events." > > nit: comments should normally end with . Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:102: * smokeDetector.addEventListener('alarm',listenerObj.someCallback,listenerObj); On 2014/04/22 02:25:35, rmsousa wrote: > This mechanism seems a bit dangerous from a lifetime management standpoint. > Javascript doesn't have weakrefs, so the objects referenced will stay alive > until they are manually removed. JS also has no way to enforce that unregister > will be called when an object is unrefrenced (e.g. if the listener's owner just > assigns a new value to the reference, and so on transitively to the owner's > owner), so we run the risk of calling an event on a very stale object that is > supposed to be gone. > > This would be fine in a one-off class, with a very definite lifetime which we > can manually inspect to ensure the events are unregistered, but feels a bit > fragile to declare a general-purpose base-class. Absolutely. You are right in the sense that objects will stay alive unless that are explicitly removed. This is especially true for DOM event listeners. The listener obj will be alive as long as DOM elements are alive. The same is also true for callbacks. Since the use of callbacks and DOM events are extensive and inevitable, I think it is the programmer's job to make sure the unhook the appropriate events when needed. There is a dispose pattern that we can use to make sure we unhook all listeners in the dispose method. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:106: base.Events = function () {}; On 2014/04/22 01:23:21, Jamie wrote: > s/Events/EventSource/? When I read Events, I assumed this was a namespace--I > think EventSource makes it clearer that it's a base/mix-in class. Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:110: * Define the events this event source On 2014/04/22 01:23:21, Jamie wrote: > Incomplete comment. Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:110: * Define the events this event source On 2014/04/22 00:43:21, Sergey Ulanov wrote: > Comments should end with '.' Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:116: var ev = this._ev = {}; On 2014/04/22 00:43:21, Sergey Ulanov wrote: > s/ev/event/ Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:121: ev[type] = { On 2014/04/22 01:23:21, Jamie wrote: > The state you're tracking for each event is somewhat subtle, so I don't think it > should be buried inside a method. Promote the EventEntry class from the proto to > be a class, and maybe give it add/remove/raise methods. > > Alternatively, I think you might be able to simplify the implementation to store > just a list of callbacks if you take a copy of that list before iterating over > it. Which approach is better depends on how many callbacks you expect there to > be. The way that events are handled here is actually a very simple data structure and I am hesitant to over-inflate it with different sub classes. I think it will make the code a bit harder to read. I can't simply make a copy of the list before raising the events. This will break the use case when you remove a listener during raiseEvents. Suppose that you have an event Foo with three listeners as follows [ onFoo1, onFoo2, onFoo3] And the listener onFoo1 is implemented as follows onFoo1 = function () { obj.removeEventListener('foo', OnFoo2, this); } If we clone the list, onFoo2 will still be fired, which is unexpected. This is the reason why we need to null out the handler if we are removing events during raiseEvents. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:129: * Define the events this event source On 2014/04/22 00:43:21, Sergey Ulanov wrote: > Update this comment Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:129: * Define the events this event source On 2014/04/22 01:23:21, Jamie wrote: > Copy/paste error? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:132: * @param {*} thisObj On 2014/04/22 01:23:21, Jamie wrote: > I think it would be better to require callers to pass in a bound function rather > than an explicit |this| pointer. Why do you think it is better to require callers to pass in a bound function? Would it be easier for the caller if he can simply pass in a this pointer and avoid necessary bindings? https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:141: * Define the events this event source On 2014/04/22 00:43:21, Sergey Ulanov wrote: > update the comment Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:141: * Define the events this event source On 2014/04/22 01:23:21, Jamie wrote: > Copy/paste error. Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:150: var listener; On 2014/04/22 00:43:21, Sergey Ulanov wrote: > does this need to be defined outside of the for loop? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:166: return; On 2014/04/22 01:23:21, Jamie wrote: > In general, I prefer to use break to exit a loop, even if there's no code > following it. It makes the function more robust to future code changes Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:182: for (var i = 0; i < listeners.length; i++) { On 2014/04/22 01:23:21, Jamie wrote: > Can you use forEach here? Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:191: for (i = listeners.length; i--;) { On 2014/04/22 00:43:21, Sergey Ulanov wrote: > The following would me more readable: > for (var i = listeners.length - 1; i >= 0; i--) > > 1) you need "- 1" > 2) I think you need "var" when defining i here. javascript is function scope. var is already declared, i can rename to j to make it more readable https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:192: if (!listeners[i]) { On 2014/04/22 00:43:21, Sergey Ulanov wrote: > Why do you need this check? Comments added https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:206: chk: function (obj, type) { On 2014/04/22 00:43:21, Sergey Ulanov wrote: > Nt rdbl nm. > > > > (not readable name) Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:207: Debug.assert(Boolean(obj._ev), 'The object doesn\'t support events'); On 2014/04/22 01:23:21, Jamie wrote: > For strings with embedded apostrophes, you can use double-quotes. Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:207: Debug.assert(Boolean(obj._ev), 'The object doesn\'t support events'); On 2014/04/22 00:43:21, Sergey Ulanov wrote: > nit: double space Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:214: Debug.assert = base.emptyFn; On 2014/04/22 00:43:21, Sergey Ulanov wrote: > Why not have asserts always on? IMO it's better to have them always on, they > just shouln't trigger alert() by default (and just log to the console) Done. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/js_proto... File remoting/webapp/js_proto/base_proto.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/js_proto... remoting/webapp/js_proto/base_proto.js:7: // JSCompiler to verify the type-correctness of our code. On 2014/04/22 01:23:21, Jamie wrote: > This should not be necessary. We have proto files for types defined externally > to the web-app, but this is code we're fully in control of. In particular, you > can get rid of the @this directive in the implementation file and just rename it > to EventSource to get the benefits of most of these directives. Done.
https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:77: * @suppress {checkTypes} On 2014/04/23 00:24:20, kelvinp wrote: > On 2014/04/22 01:23:21, Jamie wrote: > > Is it possible to make this function type-safe? > > Unfortunately, the access of src.prototype and dest.prototype really throws > JSCop off. Since this function is a one liner, I don't think we will lose too > much safety by turning type checking off here. Agreed, it's probably fine, but I'm surprised jscompiler can't handle it. Object.protoype pretty clearly has Object type so it should be happy. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:121: ev[type] = { On 2014/04/23 00:24:20, kelvinp wrote: > On 2014/04/22 01:23:21, Jamie wrote: > > The state you're tracking for each event is somewhat subtle, so I don't think > it > > should be buried inside a method. Promote the EventEntry class from the proto > to > > be a class, and maybe give it add/remove/raise methods. > > > > Alternatively, I think you might be able to simplify the implementation to > store > > just a list of callbacks if you take a copy of that list before iterating over > > it. Which approach is better depends on how many callbacks you expect there to > > be. > > The way that events are handled here is actually a very simple data structure > and I am hesitant to over-inflate it with different sub classes. I think it > will make the code a bit harder to read. > > I can't simply make a copy of the list before raising the events. This will > break the use case when you remove a listener during raiseEvents. > > Suppose that you have an event Foo with three listeners as follows > [ onFoo1, onFoo2, onFoo3] > > And the listener onFoo1 is implemented as follows > > onFoo1 = function () { > obj.removeEventListener('foo', OnFoo2, this); > } > > If we clone the list, onFoo2 will still be fired, which is unexpected. This is > the reason why we need to null out the handler if we are removing events during > raiseEvents. > As discussed, I think either approach to event handling makes sense. It's reasonable to say that an event is dispatched to all handlers that were registered when it occurred, in which case you can use the simpler approach I describe. Allowing handlers to unregister themselves is essential, but that use-case should work as expected. Allowing handlers to unregister each other is less common, and I think it's okay for it only to apply to subsequent events. https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:132: * @param {*} thisObj On 2014/04/23 00:24:20, kelvinp wrote: > On 2014/04/22 01:23:21, Jamie wrote: > > I think it would be better to require callers to pass in a bound function > rather > > than an explicit |this| pointer. > > Why do you think it is better to require callers to pass in a bound function? > Would it be easier for the caller if he can simply pass in a this pointer and > avoid necessary bindings? I think the key word is "necessary". If it's a member function, then it needs to be bound before you can invoke it, but only caller of addEventListener knows that. Requiring non-member functions to be registered with a null thisObj seems unnecessary. It's also consistent with Element.addEventListener, which requires a bound function as the callback.
I just have style nits - will let Jamie finish the review. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:15: var debugUtil = {}; maybe base.debug? https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:44: * @return {string} The callstack of the current method add . https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:66: * A utility function to invoke |obj|.dispose without a null check on |obj| . https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:105: * For example, to create an alarm event for SmokeDetector nit: add : at the end of this line https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:111: * To fire an event : https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:117: * To listen to an event : https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:119: * smokeDetector.addEventListener('alarm',listenerObj.someCallback,listenerObj); nit: spaces after ',' https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:211: /** @type {base.EventEntry}*/ here and below: add space before */ https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:217: // find the listener to remove Find the listener to remove. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:226: // if the event is unhooked during raiseEvent, we can't simply remove it nit: capital I .
https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:121: ev[type] = { On 2014/04/23 21:52:57, Jamie wrote: > On 2014/04/23 00:24:20, kelvinp wrote: > > On 2014/04/22 01:23:21, Jamie wrote: > > > The state you're tracking for each event is somewhat subtle, so I don't > think > > it > > > should be buried inside a method. Promote the EventEntry class from the > proto > > to > > > be a class, and maybe give it add/remove/raise methods. > > > > > > Alternatively, I think you might be able to simplify the implementation to > > store > > > just a list of callbacks if you take a copy of that list before iterating > over > > > it. Which approach is better depends on how many callbacks you expect there > to > > > be. > > > > The way that events are handled here is actually a very simple data structure > > and I am hesitant to over-inflate it with different sub classes. I think it > > will make the code a bit harder to read. > > > > I can't simply make a copy of the list before raising the events. This will > > break the use case when you remove a listener during raiseEvents. > > > > Suppose that you have an event Foo with three listeners as follows > > [ onFoo1, onFoo2, onFoo3] > > > > And the listener onFoo1 is implemented as follows > > > > onFoo1 = function () { > > obj.removeEventListener('foo', OnFoo2, this); > > } > > > > If we clone the list, onFoo2 will still be fired, which is unexpected. This > is > > the reason why we need to null out the handler if we are removing events > during > > raiseEvents. > > > > As discussed, I think either approach to event handling makes sense. It's > reasonable to say that an event is dispatched to all handlers that were > registered when it occurred, in which case you can use the simpler approach I > describe. Allowing handlers to unregister themselves is essential, but that > use-case should work as expected. Allowing handlers to unregister each other is > less common, and I think it's okay for it only to apply to subsequent events. It is actually not uncommon for one event handler to unhook another one. For instance, you are waiting for either for event A or event B before invoking action c. On the handler of event A or event B, you would probably unhook both events and call action c. The existing implementation ensure the correctness in this situation. I think I will just leave it as is, plus I have tested the existing code quite extensively. Switching the implementation would take extra time to re-stablize the code and I am not sure the gain is worth it https://codereview.chromium.org/245923002/diff/20001/remoting/webapp/base.js#... remoting/webapp/base.js:132: * @param {*} thisObj On 2014/04/23 21:52:57, Jamie wrote: > On 2014/04/23 00:24:20, kelvinp wrote: > > On 2014/04/22 01:23:21, Jamie wrote: > > > I think it would be better to require callers to pass in a bound function > > rather > > > than an explicit |this| pointer. > > > > Why do you think it is better to require callers to pass in a bound function? > > Would it be easier for the caller if he can simply pass in a this pointer and > > avoid necessary bindings? > > I think the key word is "necessary". If it's a member function, then it needs to > be bound before you can invoke it, but only caller of addEventListener knows > that. Requiring non-member functions to be registered with a null thisObj seems > unnecessary. It's also consistent with Element.addEventListener, which requires > a bound function as the callback. Done https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:15: var debugUtil = {}; On 2014/04/23 22:40:27, Sergey Ulanov wrote: > maybe base.debug? Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:44: * @return {string} The callstack of the current method On 2014/04/23 22:40:27, Sergey Ulanov wrote: > add . Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:66: * A utility function to invoke |obj|.dispose without a null check on |obj| On 2014/04/23 22:40:27, Sergey Ulanov wrote: > . Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:105: * For example, to create an alarm event for SmokeDetector On 2014/04/23 22:40:27, Sergey Ulanov wrote: > nit: add : at the end of this line Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:111: * To fire an event On 2014/04/23 22:40:27, Sergey Ulanov wrote: > : Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:117: * To listen to an event On 2014/04/23 22:40:27, Sergey Ulanov wrote: > : Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:119: * smokeDetector.addEventListener('alarm',listenerObj.someCallback,listenerObj); On 2014/04/23 22:40:27, Sergey Ulanov wrote: > nit: spaces after ',' Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:217: // find the listener to remove On 2014/04/23 22:40:27, Sergey Ulanov wrote: > Find the listener to remove. Done. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:226: // if the event is unhooked during raiseEvent, we can't simply remove it On 2014/04/23 22:40:27, Sergey Ulanov wrote: > nit: capital I . Done.
Looks like the latest patch set is corrupt?
On 2014/04/24 20:28:29, Wez wrote: > Looks like the latest patch set is corrupt? Are you refering to patch set 4? It looks find for me. Can you try again?
lgtm once my comments are addressed. Happy to take another look if any of them are non-trivial. https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/60001/remoting/webapp/base.js#... remoting/webapp/base.js:191: * @param {function(Object=)} fn Shouldn't this type be function, not object? Also, if you're going to make a parameter optional, it's name should begin with opt_. In this case, though, I don't think it makes sense for |fn| to be optional. https://codereview.chromium.org/245923002/diff/110001/remoting/remoting_webap... File remoting/remoting_webapp_files.gypi (right): https://codereview.chromium.org/245923002/diff/110001/remoting/remoting_webap... remoting/remoting_webapp_files.gypi:123: '<@(remoting_webapp_js_core_files)', Please keep these alphabetic. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:128: /** @type {Array.<Function>} */ Is "Function" a synonym for "function()"? https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:134: this.eventMap_ = {}; Add type? I think it should be Object.<string, EventEntry> https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:141: base.EventSource.checkType = function(obj, type) { Why not make this a regular member function? Also, checkType is a rather generic name. I suggest something like assertValidEvent. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:163: this.eventMap_[type] = { I think you want "new EventEntry" here. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:172: * invoked with |thisObj| as the this pointer. thisObj no longer exists. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:174: * @param {function(?=)} fn No need for "?=" in the type, here and below. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:180: var listeners = /** @type {Array} */ this.eventMap_[type].listeners; This cast should not be necessary if you add @type in the ctor.
LGTM once Jamie's comments are addressed. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:170: /** nit: I prefer a blank line between the }, and the start of the comment for the next case, to aid readability.
Last round of CL
https://codereview.chromium.org/245923002/diff/110001/remoting/remoting_webap... File remoting/remoting_webapp_files.gypi (right): https://codereview.chromium.org/245923002/diff/110001/remoting/remoting_webap... remoting/remoting_webapp_files.gypi:123: '<@(remoting_webapp_js_core_files)', On 2014/04/24 20:38:34, Jamie wrote: > Please keep these alphabetic. Spoke with Jamie offline. I need to put core_files on the top, as we apply the mix-in to the derived class at load time. See comment on Jscompile https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js File remoting/webapp/base.js (right): https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:128: /** @type {Array.<Function>} */ On 2014/04/24 20:38:34, Jamie wrote: > Is "Function" a synonym for "function()"? Done. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:134: this.eventMap_ = {}; On 2014/04/24 20:38:34, Jamie wrote: > Add type? I think it should be Object.<string, EventEntry> Done. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:141: base.EventSource.checkType = function(obj, type) { On 2014/04/24 20:38:34, Jamie wrote: > Why not make this a regular member function? Also, checkType is a rather generic > name. I suggest something like assertValidEvent. Renamed to isDefined. Don't want to make it a regular function as it got mixed inside the derived class. In general, I try to avoid mixing in generic names to avoid name conflicts in the derived class. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:163: this.eventMap_[type] = { On 2014/04/24 20:38:34, Jamie wrote: > I think you want "new EventEntry" here. Don't know how I miss that. Good catch https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:170: /** On 2014/04/24 20:45:15, Wez wrote: > nit: I prefer a blank line between the }, and the start of the comment for the > next case, to aid readability. Done. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:172: * invoked with |thisObj| as the this pointer. On 2014/04/24 20:38:34, Jamie wrote: > thisObj no longer exists. Done. https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:174: * @param {function(?=)} fn On 2014/04/24 20:38:34, Jamie wrote: > No need for "?=" in the type, here and below. Spoke offline. I want to indicate that the parameter to the event callback is optional https://codereview.chromium.org/245923002/diff/110001/remoting/webapp/base.js... remoting/webapp/base.js:180: var listeners = /** @type {Array} */ this.eventMap_[type].listeners; On 2014/04/24 20:38:34, Jamie wrote: > This cast should not be necessary if you add @type in the ctor. Done.
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kelvinp@chromium.org/245923002/130001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
The CQ bit was checked by kelvinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kelvinp@chromium.org/245923002/130001
Message was sent while issue was closed.
Change committed as 266164 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
