Bring events to JavaScript components

remoting/webapp/base.js

Index: remoting/webapp/base.js
diff --git a/remoting/webapp/base.js b/remoting/webapp/base.js
new file mode 100644
index 0000000000000000000000000000000000000000..074c13f61bd5422e76aeacfcec431713b6271362
--- /dev/null
+++ b/remoting/webapp/base.js
@@ -0,0 +1,214 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Sergey Ulanov, 2014/04/22 00:43:21]

lose (c).
2014

[kelvinp, 2014/04/23 00:24:20]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview
+ * A module that contains basic utility components and methods for the
+ * chromoting project
+ *
+ */
+
+'use strict';
+
+var base = {};
+var Debug = {};

[Sergey Ulanov, 2014/04/22 00:43:21]

should this be lower-case 'debug'?

[Jamie, 2014/04/22 01:23:21]

Is there a reason for the case difference between these two?

[kelvinp, 2014/04/23 00:24:20]

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

[kelvinp, 2014/04/23 00:24:20]

Done.

+
+/**
+ * Asserts that expr is true else print the msg

[Jamie, 2014/04/22 01:23:21]

s/Asserts/Assert/

[kelvinp, 2014/04/23 00:24:20]

Done.

+ * @param {boolean} expr
+ * @param {string=} opt_msg
+ */
+Debug.assert = function (expr, opt_msg) {

[Jamie, 2014/04/22 01:23:21]

Nit: No space after "function" (here and elsewhere).

[kelvinp, 2014/04/23 00:24:20]

Done.

+  if (!expr) {
+    window.alert('Assertion Failed: ' + opt_msg);
+    debugger;
+  }
+};
+
+/**
+ * @return {string} The callstack of the current method
+ * @suppress {checkTypes}

[Jamie, 2014/04/22 01:23:21]

Is it possible to make this function type-safe?

[kelvinp, 2014/04/23 00:24:20]

Done.

+ */
+Debug.callstack = function () {
+  var error = null;
+  try {
+    throw new Error();
+  } catch (e) {
+    error = e;
+  }
+  var callstack = error.stack

[Jamie, 2014/04/22 01:23:21]

Why can't this be in the catch block?

[kelvinp, 2014/04/23 00:24:20]

Done.

+    .replace(/^\s+(at eval )?at\s+/gm, '') // remove 'at' and indentation
+    .split('\n')
+    .splice(0,2); // Remove the stack of the current function
+  return callstack.join('\n');
+};
+
+/**
+ * Called on the dispose method of the obj, without needing to check for
+ * whether the object is null
+ * @param {Disposable} obj

[rmsousa, 2014/04/22 02:25:35]

Disposable isn't base JS. Are you assuming the presence of Closure (or some
equivalent framework)?

[kelvinp, 2014/04/23 00:24:20]

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

+ */
+base.dispose = function (obj) {
+  if (Boolean(obj)) {

[Jamie, 2014/04/22 01:23:21]

What does the cast to Boolean buy us in this case?

[kelvinp, 2014/04/23 00:24:20]

Trying to pre-emptively make Jscompile happy.  I guess jscompile is more content
that I thought.  removing it

+    Debug.assert(typeof obj.dispose == 'function');
+    obj.dispose();
+  }
+};
+
+/**
+ * Copies properties from the src to dest
+ * @param {Object} dest
+ * @param {Object} src
+ */
+base.mix = function(dest, src) {

[Sergey Ulanov, 2014/04/22 00:43:21]

call this copyObjectProperties()?

[kelvinp, 2014/04/23 00:24:20]

I will keep it called mix.  I think mix is shorter name and mixin is a standard
term in JavaScript 

+  for (var prop in src) {
+    if (src.hasOwnProperty(prop)) {
+      Debug.assert(!dest.hasOwnProperty(prop), 'Do not override properties');
+      dest[prop] = src[prop];
+    }
+  }
+};
+
+/**
+ * Copies properties from the src to dest.prototype

[Sergey Ulanov, 2014/04/22 00:43:21]

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"?

[kelvinp, 2014/04/23 00:24:20]

Done.

+ * @param {*} dest
+ * @param {*} src

[Jamie, 2014/04/22 01:23:21]

Why not Object for these two parameters?

[kelvinp, 2014/04/23 00:24:20]

Done.

+ * @suppress {checkTypes}

[Jamie, 2014/04/22 01:23:21]

Is it possible to make this function type-safe?

[kelvinp, 2014/04/23 00:24:20]

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. 

[Jamie, 2014/04/23 21:52:57]

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.

+ */
+base.augment = function(dest, src) {

[Sergey Ulanov, 2014/04/22 00:43:21]

call it base.addClassMixin()? and also rename the argument?

[kelvinp, 2014/04/23 00:24:20]

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.

+  base.mix(dest.prototype, src.prototype || src);
+};
+
+base.emptyFn = function () {};

[Sergey Ulanov, 2014/04/22 00:43:21]

either emptyFunction() or doNothing() 
(I would prefer latter because it's a verb and is consistent with
base::DoNothing in base/base_helpers.h).

[kelvinp, 2014/04/23 00:24:20]

Done.

+
+/**
+ * A simple implementation to augment JavaScript objects with events

[Sergey Ulanov, 2014/04/22 00:43:21]

"Mixin for classes with events."

nit: comments should normally end with .

[kelvinp, 2014/04/23 00:24:20]

Done.

+ *
+ * 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);

[rmsousa, 2014/04/22 02:25:35]

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.

[kelvinp, 2014/04/23 00:24:20]

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.

+ *
+ */
+
+base.Events = function () {};

[Jamie, 2014/04/22 01:23:21]

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.

[kelvinp, 2014/04/23 00:24:20]

Done.

+
+base.Events.prototype = {
+  /**
+    * Define the events this event source

[Sergey Ulanov, 2014/04/22 00:43:21]

Comments should end with '.'

[Jamie, 2014/04/22 01:23:21]

Incomplete comment.

[kelvinp, 2014/04/23 00:24:20]

Done.

[kelvinp, 2014/04/23 00:24:20]

Done.

+    * @param {Array.<string>} evts
+    * @this {EventSource}
+    */
+  defineEvents: function (evts) {
+    Debug.assert(!Boolean(this._ev), 'defineEvents can only be called once.');
+    var ev = this._ev = {};

[Sergey Ulanov, 2014/04/22 00:43:21]

s/ev/event/

[kelvinp, 2014/04/23 00:24:20]

Done.

+    evts.forEach(
+      /** @param {String} type*/
+      function(type) {
+        Debug.assert(typeof type == 'string');
+        ev[type] = {

[Jamie, 2014/04/22 01:23:21]

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.

[kelvinp, 2014/04/23 00:24:20]

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.

[Jamie, 2014/04/23 21:52:57]

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.

[kelvinp, 2014/04/24 01:15:39]

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 

+          recursionCount: 0,
+          sweepRequired: false,
+          listeners: []
+        };
+    });
+  },
+  /**
+    * Define the events this event source

[Sergey Ulanov, 2014/04/22 00:43:21]

Update this comment

[Jamie, 2014/04/22 01:23:21]

Copy/paste error?

[kelvinp, 2014/04/23 00:24:20]

Done.

[kelvinp, 2014/04/23 00:24:20]

Done.

+    * @param {string} type
+    * @param {function(Object=)} fn
+    * @param {*} thisObj

[Sergey Ulanov, 2014/04/22 00:43:21]

Why do we need this argument?

[Jamie, 2014/04/22 01:23:21]

I think it would be better to require callers to pass in a bound function rather
than an explicit |this| pointer.

[kelvinp, 2014/04/23 00:24:20]

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? 

[Jamie, 2014/04/23 21:52:57]

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.

[kelvinp, 2014/04/24 01:15:39]

Done

+    * @suppress {checkTypes}
+    */
+  addEventListener: function (type, fn, thisObj) {
+    Debug.assert(typeof fn == 'function');
+    Debug.Events.chk(this, type);
+    this._ev[type].listeners.push({ fn: fn, thisObj: thisObj});
+  },
+  /**
+    * Define the events this event source

[Sergey Ulanov, 2014/04/22 00:43:21]

update the comment

[Jamie, 2014/04/22 01:23:21]

Copy/paste error.

[kelvinp, 2014/04/23 00:24:20]

Done.

[kelvinp, 2014/04/23 00:24:20]

Done.

+    * @param {string} type
+    * @param {function(Object=)} fn
+    * @param {*} thisObj
+    * @suppress {checkTypes}
+    */
+  removeEventListener: function (type, fn, thisObj) {
+    Debug.assert(typeof fn == 'function');
+    Debug.Events.chk(this, type);
+    var listener;

[Sergey Ulanov, 2014/04/22 00:43:21]

does this need to be defined outside of the for loop?

[kelvinp, 2014/04/23 00:24:20]

Done.

+    var evt = this._ev[type];
+    var listeners = evt.listeners;
+    // find the listener to remove
+    for (var i = 0; i < listeners.length; i++) {
+      listener = listeners[i];
+      if (Boolean(listener) && listener.fn == fn &&
+          listener.thisObj == thisObj) {
+        // if the event is unhooked during raiseEvent, do not remove it
+        // immediately, instead set it to null and remove it later.
+        if (evt.recursionCount != 0) {
+          listeners[i] = null;
+          evt.sweepRequired = true;
+        } else {
+          listeners.splice(i, 1);
+        }
+        return;

[Jamie, 2014/04/22 01:23:21]

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

[kelvinp, 2014/04/23 00:24:20]

Done.

+      }
+    }
+  },
+  /**
+    * Fire an event of a particular type
+    * @param {string} type
+    * @param {*} details
+    * @suppress {checkTypes}
+    */
+  raiseEvent: function (type, details) {
+    Debug.Events.chk(this, type);
+    var evt = this._ev[type];
+    var listeners = evt.listeners;
+    evt.recursionCount++;
+
+    for (var i = 0; i < listeners.length; i++) {

[Jamie, 2014/04/22 01:23:21]

Can you use forEach here?

[kelvinp, 2014/04/23 00:24:20]

Done.

+      var listener = listeners[i];
+      if (listener) {
+        listener.fn.call(listener.thisObj, details);
+      }
+    }
+
+    evt.recursionCount--;
+    if (evt.recursionCount == 0 && evt.sweepRequired) {
+      for (i = listeners.length; i--;) {

[Sergey Ulanov, 2014/04/22 00:43:21]

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.

[kelvinp, 2014/04/23 00:24:20]

javascript is function scope.  var is already declared, i can rename to j to
make it more readable

+        if (!listeners[i]) {

[Sergey Ulanov, 2014/04/22 00:43:21]

Why do you need this check?

[kelvinp, 2014/04/23 00:24:20]

Comments added

+          listeners.splice(i, 1);
+        }
+      }
+      evt.sweepRequired = false;
+    }
+  }
+};
+
+Debug.Events = {
+  /**
+   * @param {EventSource} obj
+   * @param {string} type
+   */
+  chk: function (obj, type) {

[Sergey Ulanov, 2014/04/22 00:43:21]

Nt rdbl nm.



(not readable name)

[kelvinp, 2014/04/23 00:24:20]

Done.

+    Debug.assert(Boolean(obj._ev), 'The  object doesn\'t support events');

[Sergey Ulanov, 2014/04/22 00:43:21]

nit: double space 

[Jamie, 2014/04/22 01:23:21]

For strings with embedded apostrophes, you can use double-quotes.

[kelvinp, 2014/04/23 00:24:20]

Done.

[kelvinp, 2014/04/23 00:24:20]

Done.

+    Debug.assert(Boolean(obj._ev[type]), 'Event <' + type +
+      '> is undefined for the current object');
+  }
+};
+
+// Turns off assert in production. Comment out the line below for debugging.
+Debug.assert = base.emptyFn;

[Sergey Ulanov, 2014/04/22 00:43:21]

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)

[kelvinp, 2014/04/23 00:24:20]

Done.