593737 - change event-generating high-level modules to use EventEmitter event registration model

Status: RESOLVED FIXED

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

Description

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

Bug 588732 implements the EventEmitter model for event registration.  There should be one good way to register events in the SDK, so all event-generating high-level modules should be changed to use the EventEmitter model.

The implementation in bug 588732 relies on Traits, and it's unclear how usable it is with modules that use lexical scoping to hide private properties.  Nevertheless, I highly recommend that whoever takes responsibility for this bug implement the minimal changes necessary to switch modules to the EventEmitter model.

In other words, if there's a way to make the changes that doesn't require switching all those modules from lexical scope implementations to Traits implementations, we should use that way.  Even if we eventually change to using Traits everywhere, now is not the time to be doing so, as we have more than enough work to do already to get 0.8 released as our first beta.

Irakli: do you have a sense of what minimal changes are required to switch existing high-level modules using lexical scoping to the EventEmitter model?

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: example of minimal changes required for 'request' module

gist containing diff with changes: http://gist.github.com/566773

While changes here are pretty trivial here there are cases like 'tabs' where it will be more complicated, but that's a different issue, caused by the fact that code calling listeners is relying on a fact that listeners can be enumerated outside of the constructors lexical scope, which I believe had to be addressed by bug 588250 anyway.

Please also keep in mind that code is just for demo and it was not tested, it may be missing.

Comment 2

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Attachment: Applying changes disscussed

Diff with master: http://github.com/Gozala/jetpack-sdk/compare/master...events@588732

Commit:
http://github.com/Gozala/jetpack-sdk/commit/8b6c48a0bffea75eb661f8a97b4b8c6ec2a33dd9

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

please ignore my last comment I meant to attach patch to the different bug.

Comment 4

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

Irakli: is this something you could take on after finishing the ContentSymbiont work?  It's necessary in order for us to declare a beta, which makes it a high priority (more important than Sidebar).

Note: the key here is to make the minimal changes required to get high-level modules using EventEmitter-style event listener registration.  This is not the time to be rewriting all those modules to work as Traits.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

All my patches are submitted for review so I can work on this before I'll get feedback on ContentSymbiont or PageMod. Can you plese list all the modules that will need to change.

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