Closed Bug 834471 Opened 9 years ago Closed 9 years ago

Fix some FindBugs warnings

Categories

(Firefox for Android Graveyard :: General, defect, P3)

ARM
Android
defect

Tracking

(firefox20 wontfix, firefox21 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- wontfix
firefox21 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

Details

Attachments

(4 files)

GeckoInputConnection.java code compares java.lang.String objects for reference equality using the == or != operators. Consider using the equals(Object) method instead.

GeckoAppShell.java invokes inefficient new String() constructor. Creating a new java.lang.String object using the no-argument constructor wastes memory because the object so created will be functionally indistinguishable from the empty string constant "".
Attachment #706086 - Flags: review?(nchen)
DateTimePicker.java passes a constant month value outside the expected range of 0..11 to a method.

DateTimePicker.PickersState enum name should start with an upper case letter.
Attachment #706089 - Flags: review?(wjohnston)
Fix some FindBugs warnings about unused variables and make GeckoMediaScannerClient final.
Attachment #706093 - Flags: review?(wjohnston)
Fix FindBugs warning about unlockProfile() not checking file.delete() return value.
Attachment #706095 - Flags: review?(wjohnston)
Comment on attachment 706095 [details] [diff] [review]
fix-unlockProfile-warning.patch

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

::: mobile/android/base/GeckoAppShell.java
@@ +2249,5 @@
>  
>          // Then force unlock this profile
>          GeckoProfile profile = GeckoApp.mAppContext.getProfile();
>          File lock = profile.getFile(".parentlock");
> +        return lock.exists() && lock.delete();

I'm not sure we want to return false if the lock file doesn't exist here, but it looks like that's what I did before. I guess that seems "safer".
Attachment #706095 - Flags: review?(wjohnston) → review+
Comment on attachment 706089 [details] [diff] [review]
fix-DateTimePicker-warnings.patch

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

::: mobile/android/base/widget/DateTimePicker.java
@@ +220,5 @@
>              throw new UnsupportedOperationException("Custom DateTimePicker is only available for SDK > 10");
>          }
>          setCurrentLocale(Locale.getDefault());
> +        mMinDate.set(DEFAULT_START_YEAR,0,1);
> +        mMaxDate.set(DEFAULT_END_YEAR,11,31);

Probably better to just use Calendar.JANUARY/DECEMBER
Attachment #706089 - Flags: review?(wjohnston) → review+
Attachment #706093 - Flags: review?(wjohnston) → review+
Comment on attachment 706086 [details] [diff] [review]
fix-string-warnings.patch

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

::: mobile/android/base/GeckoAppShell.java
@@ +1295,5 @@
>                  } catch (InterruptedException ie) {}
>              }});
>          try {
>              String ret = sClipboardQueue.take();
> +            return ret.isEmpty() ? null : ret;

ret should never be null, right?

::: mobile/android/base/GeckoInputConnection.java
@@ +384,5 @@
>              Log.d(LOGTAG, "IME: CurrentInputMethod=" + mCurrentInputMethod);
>          }
>  
>          // If the user has changed IMEs, then notify input method observers.
> +        if (!mCurrentInputMethod.equals(prevInputMethod)) {

mCurrentInputMethod may be null?
(In reply to Jim Chen [:jchen :nchen] from comment #6)
> Comment on attachment 706086 [details] [diff] [review]
> fix-string-warnings.patch
> 
> Review of attachment 706086 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +1295,5 @@
> >                  } catch (InterruptedException ie) {}
> >              }});
> >          try {
> >              String ret = sClipboardQueue.take();
> > +            return ret.isEmpty() ? null : ret;
> 
> ret should never be null, right?

SynchronousQueue.take() can't return null. take() will throw an InterruptedException instead of returning null.


> ::: mobile/android/base/GeckoInputConnection.java
> @@ +384,5 @@
> >              Log.d(LOGTAG, "IME: CurrentInputMethod=" + mCurrentInputMethod);
> >          }
> >  
> >          // If the user has changed IMEs, then notify input method observers.
> > +        if (!mCurrentInputMethod.equals(prevInputMethod)) {
> 
> mCurrentInputMethod may be null?

mCurrentInputMethod's initial value is null. So in this code path, prevInputMethod could be null, but mCurrentInputMethod should get initialized by InputMethods.getCurrentInputMethod().

In theory, getCurrentInputMethod() *could* return null if the Secure Settings API does not have a value for Secure.DEFAULT_INPUT_METHOD. That seems very unlikely, but I'll add a check to protect us from any crazy IMEs. :)

Also, resetInputConnection() resets mCurrentInputMethod to "". Do you mind if I change that to null? mCurrentInputMethod's initial value is null, so we should already be handling the null case. Having two "no input method" values (null and "") is confusing. Or do you think I should make mCurrentInputMethod's initial value "" and ensure that mCurrentInputMethod is *never* null? I kinda like that approach.
Landed fix-DateTimePicker-warnings.patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b73d9410c6f
Whiteboard: [leave open]
Target Milestone: --- → Firefox 21
(In reply to Chris Peterson (:cpeterson) from comment #7)
> (In reply to Jim Chen [:jchen :nchen] from comment #6)
> Also, resetInputConnection() resets mCurrentInputMethod to "". Do you mind
> if I change that to null? mCurrentInputMethod's initial value is null, so we
> should already be handling the null case. Having two "no input method"
> values (null and "") is confusing. Or do you think I should make
> mCurrentInputMethod's initial value "" and ensure that mCurrentInputMethod
> is *never* null? I kinda like that approach.

You can go with the way you like; I don't have a strong opinion either way :) But I agree it should be consistent throughout.
Comment on attachment 706086 [details] [diff] [review]
fix-string-warnings.patch

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

Changing to r+ after previous comments, so you can just carry it over :)
Attachment #706086 - Flags: review?(nchen) → review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.