1142192 - Make MenuItem function-local variables final in BrowserApp.onPrepareOptionsMenu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 1

[Sheefeni Hauwanga]

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?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 3

[Sheefeni Hauwanga]

Attachment: Changed function local MenuItem variables to final for

Comment 4

[Sheefeni Hauwanga]

Attachment: Changed function local MenuItem variables to final

Comment 5

[Sheefeni Hauwanga]

Attachment: Changed function local MenuItem variables to final

Comment 6

[Sheefeni Hauwanga]

Attachment: Changed function local MenuItem variables to final.

A patch for the function local MenuItem variables to final.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c7e3c0cfd65

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 9

[Sheefeni Hauwanga]

If I'm reading it correctly it looks like they've passed. I'll go ahead and add the keyword.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.