feat(system-addon): Implement SectionsManager for custom sections #3122
Conversation
system-addon/lib/SectionsManager.jsm
Outdated
| sections: new Map(), | ||
| listeners: new Map(), // Subscribed to a specific event | ||
| feedListeners: new Set(), // Subscribed to all events | ||
| addListener(event, callback) { |
There was a problem hiding this comment.
Would it be possible to use resource://gre/modules/EventEmitter.jsm instead of reimplementing this pattern?
system-addon/lib/SectionsManager.jsm
Outdated
| const listeners = (this.listeners.get(event) || this.listeners.set(event, new Set()).get(event)); | ||
| listeners.add(callback); | ||
| }, | ||
| addFeedListener(callback) { |
There was a problem hiding this comment.
What's the distinction between addListener and addFeedListener?
system-addon/lib/SectionsManager.jsm
Outdated
| onUpdateRows({id, shouldBroadcast}) { | ||
| const {rows} = SectionsManager.sections.get(id); | ||
| if (rows) { | ||
| const action = {type: at.SECTION_ROWS_UPDATE, data: Object.assign({id}, {rows})}; |
There was a problem hiding this comment.
this can just be data: {id, rows}, no?
system-addon/lib/SectionsManager.jsm
Outdated
| this.init(); | ||
| break; | ||
| } | ||
| SectionsManager.notifyListeners(action.type); |
There was a problem hiding this comment.
I think you might want to pass the entire action for certain events – additionally, it might be easier to manage these kind of events as a single event type (maybe SectionManager.ACTION_DISPATCHED or something like that?)
AdamHillier
force-pushed
the
gh3119
branch
2 times, most recently
from
779ce11
to
ff65d68
Compare
AdamHillier
added
PR / Needs review
and removed
PR / In progress don't review yet
labels
system-addon/lib/SectionsManager.jsm
Outdated
| this.init(); | ||
| break; | ||
| } | ||
| if (SectionsManager.ACTIONS_TO_PROXY.includes(action.type)) { |
There was a problem hiding this comment.
I was going to suggest that Set might make more sense for ACTIONS_TO_PROXY here since Set.has is faster, but after running a couple of jsperf tests it looks like that's only true for large lists;.includes is actually quite a bit faster (in Firefox) for small lists.
https://jsperf.com/set-has-or-array-includes https://jsperf.com/array-includes-and-find-methods-vs-set-has
There was a problem hiding this comment.
Maybe also check SectionsManager.sections.size, since emitting won't be necessary if no sections are installed
Closes #3119