Closed Bug 598912 Opened 12 years ago Closed 12 years ago

change "notifications" module to use EventEmitter event registration model

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: irakli, Assigned: irakli)

References

()

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Assignee: nobody → rFobic
Attached patch Implements changes (obsolete) — Splinter Review
Attachment #477908 - Flags: review?(myk)
Attachment #477908 - Flags: review?(myk) → review+
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 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.
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
Attached patch v2Splinter Review
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
Closed: 12 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
You need to log in before you can comment on or make changes to this bug.