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

Status: RESOLVED FIXED

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

Description

[Mark Capella [:capella]]

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.

Comment 1

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

Hi Mark, will like to know more about this.

Comment 2

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

Attachment: bug1060354_uninitfix.diff

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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 4

[Mark Capella [:capella]]

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.

Comment 5

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

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'

Comment 6

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

Attachment: Using removeEventListeners now

Comment 7

[Mark Capella [:capella]]

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.

Comment 8

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

Attachment: Bug 1060354 - removing event listeners, r=mfinkle

Comment 9

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

Thanks Capella. I've done as said. 

I will post on irc [#mobile] about the 404 maybe in an hour or two. Thanks.

Comment 10

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

Capella, is it necessary that you assign this to me now?

Comment 11

[Jeffrey Godwyll [:jeffgodwyll] (away until Dec 15th - use "needinfo")]

This patch is ready is ready to be checked in, can someone help me.

Comment 12

[Mark Capella [:capella]]

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

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Try push looks OK, setting "checkin-needed"

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/2aa11bfbdba2

Comment 15

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/2aa11bfbdba2