Closed
Bug 1142192
Opened 9 years ago
Closed 9 years ago
Make MenuItem function-local variables final in BrowserApp.onPrepareOptionsMenu
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox40 fixed)
RESOLVED
FIXED
Firefox 40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mcomella, Assigned: shef.hauwanga, Mentored)
Details
(Whiteboard: [lang=java][good first bug])
Attachments
(1 file, 3 obsolete files)
4.29 KB,
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
We declare a lot of function-local MenuItem variables in BrowserApp.onPrepareOptionsMenu [1] - make them final! To start, set up a build environment - you can see the instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC Thanks and happy coding! ^_^ [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=676f1ad2ac2b#2929
Assignee | ||
Comment 1•9 years ago
|
||
Hello, I'm interested in taking a crack at this bug. Out of curiosity why bother making the variables final? Is it because this is a large project with lots of hands that could cause unintended changes or a stylistic (readability) choice?
Reporter | ||
Comment 2•9 years ago
|
||
Hey, Sheefeni - welcome to Bugzilla! Once you post a patch, we'll assign you to the bug so feel free to get working whenever you're ready! :) (In reply to Sheefeni Hauwanga from comment #1) > Out of curiosity why > bother making the variables final? Is it because this is a large project > with lots of hands that could cause unintended changes Right - I've even made these mistakes on small personal projects. Setting a variable as final allows the developer to make assumptions about the variable that they could not otherwise do which helps readability (no more null checks!), improves performance (no. more. null. checks! :D), and reduces the number of errors introduced into the code when a value may be mutated. These assumptions are particularly important when you are not the developer who introduced the value and are not quite sure where else it might be used (e.g. did this value, defined at line 10, change by the end of the function at line 200? Now imagine it's a member variable :P). It's not perfect in Java because only the reference is immutable (the Object at the other end is not), but it helps a bit. Many new languages even make variables immutable by default! There are probably other reasons to use final - you can find more information about this with a quick web search.
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8584171 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8584229 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 6•9 years ago
|
||
A patch for the function local MenuItem variables to final.
Assignee | ||
Updated•9 years ago
|
Attachment #8584229 -
Attachment is obsolete: true
Attachment #8584229 -
Flags: review?(michael.l.comella)
Assignee | ||
Updated•9 years ago
|
Attachment #8584181 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8584949 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8584949 -
Attachment description: patch.diff → Changed function local MenuItem variables to final.
Attachment #8584949 -
Attachment is obsolete: false
Attachment #8584949 -
Flags: review?(michael.l.comella)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → shef.hauwanga
Reporter | ||
Comment 7•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c7e3c0cfd65
Reporter | ||
Comment 8•9 years ago
|
||
Comment on attachment 8584949 [details] [diff] [review] Changed function local MenuItem variables to final. Review of attachment 8584949 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Here is a push to our try test servers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38543db71dbf Once it goes green, feel free to add the checkin-needed keyword [1]. Let me know if you need help reading the results. Note that all patches that use "checkin-needed" must also have an associated green try run. [1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree
Attachment #8584949 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 9•9 years ago
|
||
If I'm reading it correctly it looks like they've passed. I'll go ahead and add the keyword.
Keywords: checkin-needed
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2f9a553685b7
Keywords: checkin-needed
Whiteboard: [lang=java][good first bug] → [lang=java][good first bug][fixed-in-fx-team]
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2f9a553685b7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [lang=java][good first bug][fixed-in-fx-team] → [lang=java][good first bug]
Target Milestone: --- → Firefox 40
Reporter | ||
Comment 12•9 years ago
|
||
Thanks Sheefeni! If you're looking for some followup bugs, perhaps you'd like to look at bug 1085406 comment 5 for front-end work, or bug 1116351 for some background work.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•