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

RESOLVED FIXED in Firefox 35

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: capella, Assigned: jeffgodwyll, Mentored)

Tracking

unspecified
Firefox 35
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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 ...

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=3c9282756e22&mark=2755-2758#2745

... 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]
bug1060354_uninitfix.diff
Attachment #8481869 - Flags: review?(markcapella)
Comment on attachment 8481869 [details] [diff] [review]
bug1060354_uninitfix.diff

Just a drive-by:

If we decide to just remove the uninit() method, we should not try to call it here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#824

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.
(Reporter)

Comment 4

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

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:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js?rev=c8449f5148ed&mark=2877-2879#2863

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 https://wiki.mozilla.org/Mobile/Fennec/Android to build and test on my Android device, but this returns a 404 error -> "deb http://archive.ubuntu.com/ubuntu/ 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)
(Reporter)

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
(Reporter)

Comment 12

4 years ago
Before checking in to mozilla-central, we push to Try server for integration / regression testing
https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server

https://tbpl.mozilla.org/?tree=Try&rev=8441492231a5
Flags: needinfo?(markcapella)
Try push looks OK, setting "checkin-needed"
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2aa11bfbdba2
Keywords: checkin-needed
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]

Comment 15

4 years ago
https://hg.mozilla.org/mozilla-central/rev/2aa11bfbdba2
Status: NEW → RESOLVED
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.