Closed Bug 1060354 Opened 7 years ago Closed 7 years ago
Code cleanup - Correct event
Listener's that are re-added in unint() code
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'
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.
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?
This patch is ready is ready to be checked in, can someone help me.
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
Try push looks OK, setting "checkin-needed"
Whiteboard: [lang=js][good first bug] → [lang=js][good first bug][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 7 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.