551352 - Review the observer service module, jetpack-core/lib/observer-service.js

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[Drew Willcoxon :adw]

http://hg.mozilla.org/labs/jetpack-sdk/file/tip/packages/jetpack-core/lib/observer-service.js

Module description from the docs:

The observer-service module provides access to the application-wide observer service singleton.

Comment 1

[Dietrich Ayala (:dietrich)]

Marco, can you do a review pass on this module?

Comment 2

[Marco Bonardo [:mak]]

uh, i missed this request till today, sorry, it's hard to track without a targeted request...

I'll look into it soon!

Comment 3

[Marco Bonardo [:mak]]

  var service = Cc["@mozilla.org/observer-service;1"].
                getService(Ci.nsIObserverService);


Does Jetpack have something similar to Services.jsm to speed up these common services getters?
Or can it use Services.jsm?

  /**
   * A cache of observers that have been added.
   *
   * We use this to remove observers when a caller calls |Observers.remove|.
   */
  var cache = [];

name of this var is a bit generic, i think observersCache, or registeredObservers would be better.

  /**
   * Register the given callback as an observer of the given topic.
   *
   * @param   topic       {String}
   *          the topic to observe
   *
   * @param   callback    {Object}
   *          the callback; an Object that implements nsIObserver or a Function
   *          that gets called when the notification occurs

i'm not sure i like the ; usage in comments, we are used to go with periods and double spacing after them.
Is this Jetpack convention? (sorry for questions but since i'm going to work on Jetpack modules, this is useful to me too)
Also "the callback;" is a really useless comment, the param is named "callback" already.
{Object} is wrong since this could be {Object|Function}, but i don't know if Jetpack parses these definitions to build docs and if there is a possible value for multi-type params.

   *
   * @param   thisObject  {Object}  [optional]
   *          the object to use as |this| when calling a Function callback
   *
   * @returns the observer

correct javadoc syntax is @return, does Jetpack syntax differ from javadoc?

  var add = exports.add = function add(topic, callback, thisObject) {

The global coding style would want params with a preceding a, like aTopic, aCallback... is it Jetpack choice to ignore that?
if it's not, should be updated everywhere in this file

This should probably protect against misuse, like passing a null/empty topic

    var observer = new Observer(topic, callback, thisObject);
    service.addObserver(observer, topic, true);
    cache.push(observer);

I imagine addObserver could throw (rarely), and we still add our observer to the cache, maybe we should just return null in such a case and don't register it.

Also, this adds a weak observer, it could be the wanted action to avoid leaks, but i have some fear it could hide some Jetpack shutdown bug, ideally
all observers should be correctly removed on unload, so having a stong reference should not be a big deal.
Is this a choice to save against users forgetting to remove and leaving their observers in till shutdown?

  /**
   * Unregister the given callback as an observer of the given topic.
   *
   * @param   topic       {String}
   *          the topic being observed
   *
   * @param   callback    {Object}
   *          the callback doing the observing
   *
   * @param   thisObject  {Object}  [optional]
   *          the object being used as |this| when calling a Function callback
   */

same comments as above

var remove = exports.remove = function remove(topic, callback, thisObject) {

is thisObject really a way to identify an observer? topic and callback looks like enough,
are we pursuing cases where a user could register the same callback with different this objects?
that sounds like pushing users towards error-prone paths, and makes this api more complex.
Also requires them to have always available values of both the callback and thisObject to remove an observer later.
I'd vote to remove thisObject from remove()

Since addObserver returns an observer object that is created by us, could even be cool to allow to
just call remove(observer), with some sort of overloading. Would be a much easier interface to use,
and would require only to have a local reference to the created observer.
We can save any information we need for the operation in the created observer object (it looks like having those already).

    // This seems fairly inefficient, but I'm not sure how much better
    // we can make it.  We could index by topic, but we can't index by callback
    // or thisObject, as far as I know, since the keys to JavaScript hashes
    // (a.k.a. objects) can apparently only be primitive values.
    var [observer] = cache.filter(function(v) {
                                    return (v.topic      == topic    &&
                                            v.callback   == callback &&
                                            v.thisObject == thisObject);
                                  });

and would make this faster

    if (observer) {
      service.removeObserver(observer, topic);

the original API is indeed simpler than ours, and this is bad thinking to Jetpack's goals

      cache.splice(cache.indexOf(observer), 1);

at this point would have been better to use indexOf instead of filter and here we would already have the index

  /**
   * Notify observers about something.
   *
   * @param topic   {String}
   *        the topic to notify observers about
   *
   * @param subject {Object}  [optional]
   *        some information about the topic; can be any JS object or primitive
   *
   * @param data    {String}  [optional] [deprecated]
   *        some more information about the topic; deprecated as the subject
   *        is sufficient to pass all needed information to the JS observers
   *        that this module targets; if you have multiple values to pass to
   *        the observer, wrap them in an object and pass them via the subject
   *        parameter (i.e.: { foo: 1, bar: "some string", baz: myObject })
   */
  var notify = exports.notify = function notify(topic, subject, data) {

yeah, subject and data should be merged into 1 single param

    subject = (typeof subject == "undefined") ? null : new Subject(subject);
    data = (typeof    data == "undefined") ? null : data;

fancy spacing
and why not (subject === undefined) and (data === undefined) ???

  Observer.prototype = {
    QueryInterface: xpcom.utils.generateQI([Ci.nsIObserver,
                                            Ci.nsISupportsWeakReference]),

nit: I prefer 
    QueryInterface: xpcom.utils.generateQI([
      Ci.nsIObserver
    , Ci.nsISupportsWeakReference
    ]),
(or with trailing commas on each line) so that adding additional interfaces to objects doesn't pollute blame, btw, this should be done everywherem thus it's an ignorable comment

    observe: function(subject, topic, data) {
      // Extract the wrapped object for subjects that are one of our
      // wrappers around a JS object.  This way we support both wrapped
      // subjects created using this module and those that are real
      // XPCOM components.
      if (subject && typeof subject == "object" &&
          ("wrappedJSObject" in subject) &&
          ("observersModuleSubjectWrapper" in subject.wrappedJSObject))
        subject = subject.wrappedJSObject.object;

      try {
        if (typeof this.callback == "function") {
          if (this.thisObject)
            this.callback.call(this.thisObject, subject, data);
          else
            this.callback(subject, data);
        } else // typeof this.callback == "object" (nsIObserver)
          this.callback.observe(subject, topic, data);

braces for else please, since if has braces, this is unreadable
the comment should be a readable comment, not pseudo-code, otherwise use else if and throw on else (unknown callback type)

  function Subject(object) {
    // Double-wrap the object and set a property identifying the
    // wrappedJSObject as one of our wrappers to distinguish between
    // subjects that are one of our wrappers (which we should unwrap
    // when notifying our observers) and those that are real JS XPCOM
    // components (which we should pass through unaltered).
    this.wrappedJSObject = {
      observersModuleSubjectWrapper: true,
      object: object
    };
  }

  Subject.prototype = {
    QueryInterface: xpcom.utils.generateQI([]),
    getHelperForLanguage: function() {},
    getInterfaces: function() {}

this looks like an half nsIClassInfo implementation, but the QI is missing

  require("unload").when(
    function removeAllObservers() {
      // Make a copy of cache first, since cache will be changing as we
      // iterate through it.
      cache.slice().forEach(
        function(observer) {
          remove(observer.topic, observer.callback, observer.thisObject);
        });

while (cache.length) observer = cache.shift(); remove(observer); ?

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
>   var service = Cc["@mozilla.org/observer-service;1"].
>                 getService(Ci.nsIObserverService);
> 
> 
> Does Jetpack have something similar to Services.jsm to speed up these common
> services getters?
> Or can it use Services.jsm?
> 

or maybe it could provide xpcom.services.obs and so on

Comment 5

[Marco Bonardo [:mak]]

unassignin, I gave my feedback, not sure what is going to happen to this bug now.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Given the length of time this code has been in the tree, and the exposure it has received, it doesn't seem like additional review at this point is worth the cost, with the exception of the cuddlefish module, about which concerns have been raised, so closing these bugs WONTFIX, except for that one.