Fix some FindBugs warnings

RESOLVED FIXED in Firefox 21

Status

()

Firefox for Android
General
P3
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
Firefox 21
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox20 wontfix, firefox21 fixed)

Details

Attachments

(4 attachments)

(Assignee)

Description

5 years ago
Created attachment 706086 [details] [diff] [review]
fix-string-warnings.patch

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)
(Assignee)

Comment 1

5 years ago
Created attachment 706089 [details] [diff] [review]
fix-DateTimePicker-warnings.patch

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)
(Assignee)

Comment 2

5 years ago
Created attachment 706093 [details] [diff] [review]
fix-scanMedia-warnings.patch

Fix some FindBugs warnings about unused variables and make GeckoMediaScannerClient final.
Attachment #706093 - Flags: review?(wjohnston)
(Assignee)

Comment 3

5 years ago
Created attachment 706095 [details] [diff] [review]
fix-unlockProfile-warning.patch

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?
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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+
You need to log in before you can comment on or make changes to this bug.