845612 - Override All The Things

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: Add missing @Override annotations

We should use @Override whenever possible to make overriding explicit. This helps make the code more readable, and it also reduces the potential for errors if methods are renamed or changed.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Add @Overrides annotations for implemented interfaces

It's also good practice to use @Override for implemented interface methods, so this patch does that.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718771 [details] [diff] [review]
Add missing @Override annotations

Review of attachment 718771 [details] [diff] [review]:
-----------------------------------------------------------------

\o/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 718816 [details] [diff] [review]
Add @Overrides annotations for implemented interfaces

Review of attachment 718816 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure this is really needed since typoing these function names will result in a compilation failure anyway. But r+ since you've already done all the work :)

::: mobile/android/base/FindInPageBar.java
@@ +68,5 @@
>          } else {
>              // showSoftInput won't work until after the window is focused.
>              mFindText.setOnWindowFocusChangeListener(new CustomEditText.OnWindowFocusChangeListener() {
> +               @Override
> +            public void onWindowFocusChanged(boolean hasFocus) {

indentation

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 718816 [details] [diff] [review]
> Add @Overrides annotations for implemented interfaces
> 
> Review of attachment 718816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not sure this is really needed since typoing these function names will
> result in a compilation failure anyway. But r+ since you've already done all
> the work :)

Yeah, it does seem a bit overkill in certain places (especially when implementing single method interfaces like Runnable.run()). But there are some cases where it should help readability; namely, anywhere we lump in interface implementations with other methods, like http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#156 or http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/TabsTray.java#259, since it's not obvious just from glancing at the code.

In case you're curious, I made these patches using Eclipse's code cleanup tools (thanks to romaxa's Eclipse project generation scripts).

Comment 5

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed22faa72a82
https://hg.mozilla.org/integration/mozilla-inbound/rev/7164bd42720f

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ed22faa72a82
https://hg.mozilla.org/mozilla-central/rev/7164bd42720f