[Code Review] Trying to wrap my head around promises. Here's my attempt at rewriting one somewhat nested function. I'm not sure how to stop propagation from within a promise callback so, for the rewritten version, I changed `this.controller.isActive` to reject the promise if `isActive` is `false`. I don't really know if this code even works, I w…

original.js

ProfilerPanel.prototype = {
  stopProfiling: function () {
    this.controller.isActive(function (err, isActive) {
      if (err) {
        Cu.reportError("ProfilerController.isActive: " + err.message);
        return;
      }

      if (!isActive) {
        return;
      }

      this.controller.stop(function (err, data) {
        if (err) {
          Cu.reportError("ProfilerController.stop: " + err.message);
          return;
        }

        this.activeProfile.parse(data, function onParsed() {
          this.emit("parsed");
        }.bind(this));

        onStop();
        this.emit("stopped");
      }.bind(this));
    }.bind(this));
  }
};

rewrite.js

ProfilerPanel.prototype = {
  stopProfiling: function () {
    let {emit, controller, activeProfile} = this;
    let isActive = controller.isActive();

    let stop = function () {
      return controller.stop().then(function emit("stopped"));
    };

    let parse = function (data) {
      activeProfile.parse(data, function emit("parsed"));
    }
    
    isActive.then(stop).then(parse).then(onStop, function (err) {
      if (err) {
        Cu.reportError("ProfilerController.stopProfiling: " + err.message);
      }
    });
  }
};

I think part of your problem is that an asynchronous function like isActive is just bizarre. Instead, I think it makes sense for stop() to return a promise which is resolved as soon as the profiler is stopped (i.e. it's already resolved if the profiler was already stopped when stop() was called)

So:

ProfilerPanel.prototype = {
  stopProfiling: function () {
    return this.controller.stop().then(function(data) {
      // I'm going to assume you need these as backwards compat, but you could
      // or probably should, remove them otherwise.
      onStop();
      this.emit("stopped");

      // Presumably ProfilerPanel.stopProfiling is expected complete only when
      // profile is parsed too?
      return this.activeProfile.parse(data).then(function(parsedData) {
        // See above
        this.emit("parsed");

        return parsedData;
      }.bind(this));
    }.bind(this));
  }
};

// Usage:

profilerPanel.stopProfiling().then(function(parsedData) {
  // At this point the profilers is stopped and the profile is parsed
  console.log(parsedData);
});

Kind of off-topic, but I'm a bit confused by this:

    let stop = function () {
      return controller.stop().then(function emit("stopped"));
    };

I'm not sure if that's just a typo or some ES6 syntax that I'm not aware of... I'm thinking what you mean here is:

    let stop = function () {
      return controller.stop().then(emit.bind(this, "stopped"));
    };

@joewalker, this is true—isActive is bizarre. Thanks!
@jugglinmike, that's a Firefox specific shortcut syntax for one-statement functions. Don't know if its in ES6 or not.

The if (err) should not be necessary, unless one of your steps is intentionally doing throw undefined or similar.

@domenic if (err) is to distinguish between the case when isActive fails and the case when it simply returns false. It's unnecessary if I move async isActive into the controller.stop.