Make MenuItem function-local variables final in BrowserApp.onPrepareOptionsMenu

RESOLVED FIXED in Firefox 40

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: Sheefeni Hauwanga, Mentored)

Tracking

unspecified
Firefox 40
All
Android
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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

3 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?
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

3 years ago
Created attachment 8584171 [details] [diff] [review]
Changed function local MenuItem variables to final for
(Assignee)

Comment 4

3 years ago
Created attachment 8584181 [details] [diff] [review]
Changed function local MenuItem variables to final
(Assignee)

Updated

3 years ago
Attachment #8584171 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
Created attachment 8584229 [details] [diff] [review]
Changed function local MenuItem variables to final
Attachment #8584229 - Flags: review?(michael.l.comella)
(Assignee)

Comment 6

3 years ago
Created attachment 8584949 [details] [diff] [review]
Changed function local MenuItem variables to final.

A patch for the function local MenuItem variables to final.
(Assignee)

Updated

3 years ago
Attachment #8584229 - Attachment is obsolete: true
Attachment #8584229 - Flags: review?(michael.l.comella)
(Assignee)

Updated

3 years ago
Attachment #8584181 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8584949 - Attachment is obsolete: true
(Assignee)

Updated

3 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)
Assignee: nobody → shef.hauwanga
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c7e3c0cfd65
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

3 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
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]
https://hg.mozilla.org/mozilla-central/rev/2f9a553685b7
Status: NEW → RESOLVED
Last Resolved: 3 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
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.
You need to log in before you can comment on or make changes to this bug.