Code cleanup - Correct eventListener's that are re-added in unint() code

RESOLVED FIXED in Firefox 35



4 years ago
4 years ago


(Reporter: capella, Assigned: jeffgodwyll, Mentored)


Firefox 35

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [lang=js][good first bug])


(1 attachment, 2 obsolete attachments)



4 years ago
Much of the code and UI for Firefox / Android is Java, but a lot of support code is Gecko / javascript still.

Here ...

... we seem to be re-adding eventListeners in uninit() code where we need to remove them.
Hi Mark, will like to know more about this.
Created attachment 8481869 [details] [diff] [review]
Attachment #8481869 - Flags: review?(markcapella)
Comment on attachment 8481869 [details] [diff] [review]

Just a drive-by:

If we decide to just remove the uninit() method, we should not try to call it here:

As for removing the method vs. changing BrowserApp.deck.addEventListener to BrowserApp.deck.removeEventListener, I see the desktop Firefox doesn't seem to ever call removeEventListener, so maybe that is viable for Fennec too.

Comment 4

4 years ago
Comment on attachment 8481869 [details] [diff] [review]

Review of attachment 8481869 [details] [diff] [review]:

For the purpose of this bug, the intention is simply to modify the LightWeightThemeWebInstaller's uninit() method to remove the listeners that are added in it's init() method.

This would be consistent with how we add/remove observers during init()/uninit() of other objects such as DesktopUserAgent, for example:

Jeff, do you have an Android device? Are you building and testing/exploring your patch? While probably not strictly required for completion of this simple patch, it's a worthwhile effort if you intend to continue contributing in this area. Let me know and I'll point out some resources, or find me on irc in #introduction or #mobile.

I'd like to reserve consideration of what mfinkle points out, the possibility that we could remove the uninit() method entirely, for a follow-on bug. If we go there, I'd personally have to wonder further why we even need /any/ of the uninit() methods in the BrowserApp.shutdown() method.
Attachment #8481869 - Flags: review?(markcapella) → feedback+
Thanks for the feedback Mark and Mark ;). 

Capella, I followed intructions at to build and test on my Android device, but this returns a 404 error -> "deb raring main restricted universe multiverse"
apt-get update
So I'm unable to 'apt-get install -y ia32-libs'
Created attachment 8481964 [details] [diff] [review]
Using removeEventListeners now
Attachment #8481869 - Attachment is obsolete: true
Attachment #8481964 - Flags: review?(markcapella)

Comment 7

4 years ago
Comment on attachment 8481964 [details] [diff] [review]
Using removeEventListeners now

Review of attachment 8481964 [details] [diff] [review]:

I'm not sure how you're getting a 404 there (?) Are you adding that |deb| line to the end of your sources file or trying to execute it? (irc might be easier for this)

For your patch we'll be asking mfinkle for final approval, so you'll need to update your commit message to be like:
Bug 1060354 - removing event listeners, r=mfinkle

Also, the patch looks like it's a diff against your previous diff, versus a combined patch that would show both the |-| removed lines and the |+| replaced lines. Do you have multiple patches in your queue? You can put them together using the |hg qfold| command, or just create a new patch from scratch, whichever is easier / faster for you.
Attachment #8481964 - Flags: review?(markcapella)
Created attachment 8481983 [details] [diff] [review]
Bug 1060354 - removing event listeners, r=mfinkle
Attachment #8481964 - Attachment is obsolete: true
Attachment #8481983 - Flags: review?(mark.finkle)
Thanks Capella. I've done as said. 

I will post on irc [#mobile] about the 404 maybe in an hour or two. Thanks.
Attachment #8481983 - Flags: review?(mark.finkle) → review+
Capella, is it necessary that you assign this to me now?
Flags: needinfo?(markcapella)
This patch is ready is ready to be checked in, can someone help me.
Assignee: nobody → jeffgodwyll

Comment 12

4 years ago
Before checking in to mozilla-central, we push to Try server for integration / regression testing
Flags: needinfo?(markcapella)
Try push looks OK, setting "checkin-needed"
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]

Comment 15

4 years ago
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.