616770 - Convert simple-storage events to style described in bug 593921

Status: RESOLVED FIXED

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

Description

[Drew Willcoxon :adw]

Attachment: patch

See bug 593921 comment 8.

Comment 1

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

Comment on attachment 495331 [details] [diff] [review]
patch

Note that the `this` object in OverQuota handlers is currently the `manager` object, whereas it should be the `exports` object, since that is the object to which one adds the handler, and to avoid exposing the `manager` object to API consumers.

Offhand, however, I don't see a way to fix that.  EventEmitter._emit just uses this._public.  Perhaps it can be made to accept a thisObject parameter?

Comment 2

[Drew Willcoxon :adw]

Attachment: patch 2

Yes.  It's my fault, I should have caught it in review.

Modifying _emit to take a thisArg, although it would be easy to do, would punch a hole in the traits pattern.  In the long run I think that might cause confusion and encourage trading good class design for easy hacks.  The real problem is that exports can't be an EventEmitter because it can't be set to a new object.

So I made this even worse hack, but it's limited to simple-storage, and it works.  Maybe I'm being too rigid about _emit and a thisArg.  I'm willing to bend!

Comment 3

[Drew Willcoxon :adw]

The selection module faces this same problem...  Modifying _emit to take a thisArg is the better way to go I guess, but another reason that'll suck is because it's variadic; all its callers will have to be modified too.

Comment 4

[Drew Willcoxon :adw]

Or, there could be another function _emitOnObject that takes a thisArg and delegates to _emit -- or the other way around I guess.  _emitOnObject would only be used where needed.

Comment 5

[Drew Willcoxon :adw]

Attachment: patch 3

See comments 3 and 4.

Comment 6

[Drew Willcoxon :adw]

I wonder if there should be a version of _emit specifically for high-level APIs that always passes a single object to listeners that automatically includes emitter: targetObj?

Comment 7

[Drew Willcoxon :adw]

Attachment: alternate _emitEventObject and _emitMessage patch

This adds _emitEventObject like comment 6 says.  For high-level APIs that don't need to fuss with the `this` issue, they can just do:

  _emitEventObject("FooEvent");
  _emitEventObject("BarEvent", { data: someMessage });

Those that do need to fuss can pass an extra arg to set `this`.  Either way the single object that listeners receive automatically has `emitter` set.

While I was there an _emitMessage seemed like it'd be useful, so I made that too.  It automatically sets `data` to the message and works similarly to _emitEventObject otherwise.

Comes with tests.

Comment 8

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

Comment on attachment 495419 [details] [diff] [review]
alternate _emitEventObject and _emitMessage patch

> So I made this even worse hack, but it's limited to simple-storage, and it
> works.  Maybe I'm being too rigid about _emit and a thisArg.  I'm willing to
> bend!

Hacks are ok as long as they're safe enough and understood to be temporary!

Note bug 577782, which should make it possible for an exports object to be an event emitter.

This patch seems like a good approach to me and looks/works great. r=myk!

Comment 9

[Drew Willcoxon :adw]

Thanks.  I added a comment about bug 577782 to _emitOnObject.

https://github.com/mozilla/addon-sdk/commit/7e05d2e8fca1e21343aa681de77b837373346cc1

https://github.com/mozilla/addon-sdk/commit/e550db0b396b1140932f796cbd0a0c2fbe6e9a7d

https://github.com/mozilla/addon-sdk/commit/d0711ca1906172c837336151ad8bc817407606c4

Comment 10

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

https://github.com/mozilla/addon-sdk/commit/1fd44c34878bb2f27a15eb15784f7db68b9c9703
https://github.com/mozilla/addon-sdk/commit/be696bccd3a4435b939b8e21ef088bcc88e40863
https://github.com/mozilla/addon-sdk/commit/a8ea8884d30b46b340a1efed824c9d6967967077