Closed Bug 1060354 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: capella, Assigned: jeffgodwyll, Mentored)

Details

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

Attachments

(1 file, 2 obsolete files)

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.
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.
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'
Attached patch Using removeEventListeners now (obsolete) — Splinter Review
Attachment #8481869 - Attachment is obsolete: true
Attachment #8481964 - Flags: review?(markcapella)
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)
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
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]
https://hg.mozilla.org/mozilla-central/rev/2aa11bfbdba2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][good first bug][fixed-in-fx-team] → [lang=js][good first bug]
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: