Convert notification events to style described in bug 593921

RESOLVED FIXED in 1.0b1

Status

Add-on SDK
General
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

unspecified
1.0b1

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [cherry-pick-1.0b1])

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
Created attachment 495327 [details] [diff] [review]
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?
Attachment #495327 - Flags: review?(myk)
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.
Attachment #495327 - Flags: review?(myk) → review+
(Assignee)

Comment 2

7 years ago
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?
(Assignee)

Comment 3

7 years ago
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?
(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!
(Assignee)

Comment 5

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

follow-up for `this` issue:
https://github.com/mozilla/addon-sdk/commit/1234a6d224c8e9825b6a37b898b163241bb9da37
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [cherry-pick-needed]
Target Milestone: -- → 0.10
https://github.com/mozilla/addon-sdk/commit/07525c963038690f155974c0e6ee6052b14b5ab9
https://github.com/mozilla/addon-sdk/commit/ade388dfa138685df7f340c53847bd195b65a982
Whiteboard: [cherry-pick-needed] → [cherry-pick-1.0b1]
Target Milestone: 0.10 → 1.0b1
You need to log in before you can comment on or make changes to this bug.