616766 - Convert notification 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.

Currently the options object passed to the ctor is being passed as the "emitter" to onClick.  It's not true that the options object is the emitter, and besides, there are no notification objects in this API which would emit, only the global function.  So I've left out the emitter property.  If anything, the module itself is the emitter.  (And at some point the module should be a real EventEmitter so that on() and removeListener() are available.)   What do you think?

Comment 1

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

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

(In reply to comment #0)
> Currently the options object passed to the ctor is being passed as the
> "emitter" to onClick.  It's not true that the options object is the emitter,
> and besides, there are no notification objects in this API which would emit,
> only the global function.  So I've left out the emitter property.  If anything,
> the module itself is the emitter.  (And at some point the module should be a
> real EventEmitter so that on() and removeListener() are available.)   What do
> you think?

Yup, that makes sense.  The `options` object is just being used to provide named parameters.  And we can make the module itself be the emitter (and a real EventEmitter) in a future change.

However, in that case, the options object shouldn't be the `this` object either.  Instead, the `this` object should be null (with a note that it should eventually be made to be the notifications module itself). r=myk with that change.

Comment 2

[Drew Willcoxon :adw]

Hmm, there's no way (is there?) to set `this` to null.  call()ing or apply()ing with null only sets `this` to the global scope in which call() or apply() are called, in this case the sandbox.

So I think it should be set to exports, i.e., the notifications module, especially since it's clear that the module should eventually be a real EventEmitter and the emitter of the click event.  How's that sound?

Comment 3

[Drew Willcoxon :adw]

Sorry, you weren't CC'ed:

> Hmm, there's no way (is there?) to set `this` to null.  call()ing or apply()ing
> with null only sets `this` to the global scope in which call() or apply() are
> called, in this case the sandbox.
> 
> So I think it should be set to exports, i.e., the notifications module,
> especially since it's clear that the module should eventually be a real
> EventEmitter and the emitter of the click event.  How's that sound?

Comment 4

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

(In reply to comment #3)

> > So I think it should be set to exports, i.e., the notifications module,
> > especially since it's clear that the module should eventually be a real
> > EventEmitter and the emitter of the click event.  How's that sound?

Sounds good!

Comment 5

[Drew Willcoxon :adw]

initial patch:
https://github.com/mozilla/addon-sdk/commit/011c945a4e3681f82f45ce44239af42268767b9b

follow-up for `this` issue:
https://github.com/mozilla/addon-sdk/commit/1234a6d224c8e9825b6a37b898b163241bb9da37

Comment 6

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

https://github.com/mozilla/addon-sdk/commit/07525c963038690f155974c0e6ee6052b14b5ab9
https://github.com/mozilla/addon-sdk/commit/ade388dfa138685df7f340c53847bd195b65a982