Closed Bug 845612 Opened 8 years ago Closed 8 years ago

Override All The Things

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: bnicholson, Assigned: bnicholson)

Details

Attachments

(2 files)

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.
Attachment #718771 - Flags: review?(bugmail.mozilla)
It's also good practice to use @Override for implemented interface methods, so this patch does that.
Attachment #718816 - Flags: review?(bugmail.mozilla)
Comment on attachment 718771 [details] [diff] [review]
Add missing @Override annotations

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

\o/
Attachment #718771 - Flags: review?(bugmail.mozilla) → review+
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
Attachment #718816 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
https://hg.mozilla.org/mozilla-central/rev/ed22faa72a82
https://hg.mozilla.org/mozilla-central/rev/7164bd42720f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.