change "notifications" module to use EventEmitter event registration model

RESOLVED FIXED

Status

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

People

(Reporter: irakli, Assigned: irakli)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Assignee: nobody → rFobic
Created attachment 477908 [details] [diff] [review]
Implements changes
Attachment #477908 - Flags: review?(myk)
Attachment #477908 - Flags: review?(myk) → review+
Keywords: checkin-needed

Comment 3

8 years ago
Wait, why are you changing `this` to be the data?  `this` should be the options object to which the data and onClick are attached.

Comment 4

8 years ago
Comment on attachment 477908 [details] [diff] [review]
Implements changes

The patch itself is not good, because it doesn't update the doc.
Attachment #477908 - Flags: review-
Comment on attachment 477908 [details] [diff] [review]
Implements changes

(In reply to comment #3)
> Wait, why are you changing `this` to be the data?  `this` should be the options
> object to which the data and onClick are attached.

Erm, right, sorry, I missed that.  Thus this still needs to |call| the callback to set `this` appropriately.
Attachment #477908 - Flags: review+ → review-
Maybe I'm missing something, but change of not using |call| was intentional since that's a way `EventEmitter` works.
(In reply to comment #4)
> Comment on attachment 477908 [details] [diff] [review]
> Implements changes
> 
> The patch itself is not good, because it doesn't update the doc.

There is actually nothing to change on the documentation side that's why I don't.

Comment 8

8 years ago
How is changing the meaning of `this` inside onClick related to EventEmitter?  Why would `this` be anything other than the object on which I've defined onClick?

The documentation explicitly mentions what arguments onClick will be passed and this.data.
Sorry Drew it seems that I misunderstood what switching to `EventEmitter` model meant. My understanding was that we'll be sticking to original interface they way it is implemented in browser, nodejs, etc.. But it seems we're not and in that case no change required at all.

If you want to know why IMO binding this is wrong:
https://bugzilla.mozilla.org/show_bug.cgi?id=598983#c3

Now I see that the example in the docs indeed mentions this.data
Created attachment 481258 [details] [diff] [review]
v2

Updating implementation according to the comments.
Attachment #477908 - Attachment is obsolete: true
Attachment #481258 - Flags: review?(myk)
Attachment #481258 - Flags: review?(myk) → review+
c823f5a6d14b
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.