834471 - Fix some FindBugs warnings

Status: RESOLVED FIXED

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

Description

[Chris Peterson [:cpeterson]]

Attachment: 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 "".

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: 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.

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: fix-scanMedia-warnings.patch

Fix some FindBugs warnings about unused variables and make GeckoMediaScannerClient final.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: fix-unlockProfile-warning.patch

Fix FindBugs warning about unlockProfile() not checking file.delete() return value.

Comment 4

[Wesley Johnston (:wesj)]

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".

Comment 5

[Wesley Johnston (:wesj)]

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

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

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?

Comment 7

[Chris Peterson [:cpeterson]]

(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.

Comment 8

[Chris Peterson [:cpeterson]]

Landed fix-DateTimePicker-warnings.patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b73d9410c6f

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

(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 10

[(inactive) Jim Chen [:jchen] [:darchons]]

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 :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9b73d9410c6f

Comment 12

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e67cde12f976
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e83bcb87f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/528f5373b47a

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e67cde12f976
https://hg.mozilla.org/mozilla-central/rev/23e83bcb87f8
https://hg.mozilla.org/mozilla-central/rev/528f5373b47a