AddonManager.jsm's typeListeners addonListeners and managerListeners arrays should be Sets

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Add-ons Manager
P5
normal
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: florian, Assigned: tiago, Mentored)

Tracking

({good-first-bug})

unspecified
mozilla56
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
The code currently uses surprisingly complicated code to check for duplicates in these arrays:

    if (!this.managerListeners.some(i => i == aListener))
      this.managerListeners.push(aListener);

And the removal code is also surprising:

    let pos = 0;
    while (pos < this.managerListeners.length) {
      if (this.managerListeners[pos] == aListener)
        this.managerListeners.splice(pos, 1);
      else
        pos++;
    }
I agree, this would be a decent thing to cleanup and shouldn't break any externally exposed APIs.
Keywords: good-first-bug

Updated

a year ago
Priority: -- → P5
Whiteboard: triaged
Mentor: aswan
Comment hidden (mozreview-request)
First glance looks good, I started a try run for you:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b59e686d4f9c
Assignee: nobody → tiago.paez11

Comment 4

a year ago
mozreview-review
Comment on attachment 8869308 [details]
Bug 1354186 - Change managerListeners, installListeners, addonListeners and typeListeners from Array to Set.

https://reviewboard.mozilla.org/r/140862/#review144658

Looks good, thanks!
Attachment #8869308 - Flags: review?(aswan) → review+

Comment 5

a year ago
Thanks for patch. Once you've got a review+, you can add in the keyword, "checkin-needed" and it will attempt to be merged to the tree. I've done this on your behalf, so we'll see how it goes.
Keywords: checkin-needed

Comment 6

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/287690a1fedc
Change managerListeners, installListeners, addonListeners and typeListeners from Array to Set. r=aswan
Keywords: checkin-needed

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/287690a1fedc
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Thanks for the patch, Santiago! Your contribution has been added to our recognition wiki here: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#June_2017

Your Mozillians profile has also been vouched. :) 

Welcome onboard! I look forward to seeing you around.
Duplicate of this bug: 852813
You need to log in before you can comment on or make changes to this bug.