Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this?

(In reply to Reece Martin from comment #1)

Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this?

Hi Reece, thanks for your help! I can assign this bug to you after you post a patch.

Have you built the "GeckoViewExample" test app yet? Downloading the code and build it are the first steps. Here are instructions:

https://mozilla.github.io/geckoview/contributor/geckoview-quick-start

If you have questions, you can ask in this bug report or join the #mobile channel on Mozilla's IRC server:

https://wiki.mozilla.org/IRC

If you don't have an IRC client, IRCCloud is a free and nice web client:

https://www.irccloud.com/

(In reply to Chris Peterson [:cpeterson] from comment #2)

(In reply to Reece Martin from comment #1)

Hi there! I am a University of Toronto compsci student interested in picking this bug up as my first Mozilla bug. Can I claim this?

Hi Reece, thanks for your help! I can assign this bug to you after you post a patch.

Have you built the "GeckoViewExample" test app yet? Downloading the code and build it are the first steps. Here are instructions:

https://mozilla.github.io/geckoview/contributor/geckoview-quick-start

If you have questions, you can ask in this bug report or join the #mobile channel on Mozilla's IRC server:

https://wiki.mozilla.org/IRC

If you don't have an IRC client, IRCCloud is a free and nice web client:

https://www.irccloud.com/

Sounds good Chris, I'll get on it.

Trying to get started on this bug, and there are a couple of things I am stuck on/confused about:

I went into this bug thinking that it is simply a matter of negating the output of each affected method, then negating all its use cases, but when I look at the actual methods, they are things like an update method where it makes sense to return true when the operation completes successfully and false when the operation fails. Does it still make sense to negate them?

Also, I didn't find any test cases for this section of code, does that mean I have to write some myself to test these?

(In reply to Reece Martin from comment #4)

I went into this bug thinking that it is simply a matter of negating the output of each affected method, then negating all its use cases, but when I look at the actual methods, they are things like an update method where it makes sense to return true when the operation completes successfully and false when the operation fails. Does it still make sense to negate them?

Can you share an example of such a method? Perhaps we don't need to change all the reported methods or the method can be changed to return a more explicit success/failure type using an enum instead of a boolean. Methods with boolean parameters or return values are often a "code smell" because a method with two different code paths for a special case often needs to handle more special cases in the future. :)

Also, I didn't find any test cases for this section of code, does that mean I have to write some myself to test these?

If there are no tests for this code, then I don't think you need to write new tests... unless you would like to. :) Running and writing tests will add a lot of new challenges if this is your first Firefox bug.

Emily, what tool are you using to detect these inverted boolean methods?

(In reply to Chris Peterson [:cpeterson] from comment #5)

(In reply to Reece Martin from comment #4)

I went into this bug thinking that it is simply a matter of negating the output of each affected method, then negating all its use cases, but when I look at the actual methods, they are things like an update method where it makes sense to return true when the operation completes successfully and false when the operation fails. Does it still make sense to negate them?

Can you share an example of such a method? Perhaps we don't need to change all the reported methods or the method can be changed to return a more explicit success/failure type using an enum instead of a boolean. Methods with boolean parameters or return values are often a "code smell" because a method with two different code paths for a special case often needs to handle more special cases in the future. :)

One example would be the Class SplineStackScroller in the StackScroller.java file in the package mozilla.gecko.gfx, with the method update that returns true if the update has been done and false if it wasn't done. There is also a boolean field in this same class mFinished that represents whether animation is currently in progress or not, and this was flagged as well.

Also, I didn't find any test cases for this section of code, does that mean I have to write some myself to test these?

If there are no tests for this code, then I don't think you need to write new tests... unless you would like to. :) Running and writing tests will add a lot of new challenges if this is your first Firefox bug.

Got it, thanks!

I've just done a check for all the methods that were flagged, and most were in fact methods that seem appropriate to return the boolean value as they do now with some acting as an accessor to a private variable. Here is a select sample of method names for reference:

• isSupported
• shouldInitCodec
• isInstance
• contains
• isEmpty
• equals
• shouldProcessKey

I don't believe these should be simply inverted, and I'm not sure if using an enum type to return a simple boolean like this is necessary.

If I should change them to enum return types, how should I go about doing it? Creating a type for every single method involved?

I've just done a check for all the methods that were flagged, and most were in fact methods that seem appropriate to return the boolean value as they do now with some acting as an accessor to a private variable.
...
I don't believe these should be simply inverted, and I'm not sure if using an enum type to return a simple boolean like this is necessary.

Interesting. Sounds like this Java warning is not useful if it hasn't found any real bugs or confusing code. In that case, perhaps the appropriate fix for this bug is a code patch that disables this warning. :) Is this warning reported by javac? Is there is a command-line option to disable this warning?

(In reply to Chris Peterson [:cpeterson] from comment #8)

Interesting. Sounds like this Java warning is not useful if it hasn't found any real bugs or confusing code. In that case, perhaps the appropriate fix for this bug is a code patch that disables this warning. :) Is this warning reported by javac? Is there is a command-line option to disable this warning?

I just did a rebuild using Mach and I did not see any of these warnings being reported in the console (the only ones reported were C and C++ warnings), so I doubt there is an option to disable these unless I am doing something wrong with the build.

Looks like this warning is from the Android Studio IDE, not javac.

https://www.jetbrains.com/help/idea/2016.1/invert-boolean.html
https://stackoverflow.com/questions/52648487/suppresswarnings-for-boolean-method-methodname-is-always-inverted

The warnings can be suppressed with a method annotation like @SuppressWarnings("BooleanMethodIsAlwaysInverted"). There's probably a way to disable them globally, but suppressing them for each method is good place to start. Here are some code examples from a random project:

https://github.com/jonasoreland/runnerup/pull/524/commits/073bb9cf1560b581169d9de521c164a724a8f39c

Correction: I spoke with the GeckoView developers and they think we should just disable this "BooleanMethodIsAlwaysInverted" warning globally instead of annotating the methods with warnings.

So your patch could create a new lint.xml file to disable this warning. We can use lint.xml to disable other bogus warnings in the future. Here are Android Studio instructions for using lint.xml:

https://developer.android.com/studio/write/lint.html#sample-lint.xml-file

I did some digging around trying to make this work, but unfortunately, I've run into a few problems. Since this specific warning comes from the Android Studio/IntelliJ inspector instead of the Android linter, lint.xml is unable to disable the warning (it tells me that it does not recognize the issue ID "BooleanMethodIsAlwaysInverted". I tried digging around in the inspector, but besides being able to save the setting locally as well as exporting the setting (which requires manual import in order to be applied), there is no way to actually have it saved as a permanent setting in the repository. I'll try digging around some more, but if the GeckoView developers are okay with SuppressWarnings annotations, I can go in that route as well.

(In reply to Reece Martin from comment #12)

I did some digging around trying to make this work, but unfortunately, I've run into a few problems. Since this specific warning comes from the Android Studio/IntelliJ inspector instead of the Android linter, lint.xml is unable to disable the warning (it tells me that it does not recognize the issue ID "BooleanMethodIsAlwaysInverted". I tried digging around in the inspector, but besides being able to save the setting locally as well as exporting the setting (which requires manual import in order to be applied), there is no way to actually have it saved as a permanent setting in the repository.

I don't have Android Studio, so I can't help here. Where does the Android Studio save your local setting? I would be surprised there isn't a way to save Android Studio settings to a file that can be checked into version control.

This Android doc page talks about disabling lint checks from Gradle. Perhaps that is an option?

https://developer.android.com/studio/write/lint#gradle

I'll try digging around some more, but if the GeckoView developers are okay with SuppressWarnings annotations, I can go in that route as well.

I think suppressing the warnings using @SuppressWarnings("BooleanMethodIsAlwaysInverted") annotations is much better than leaving the warnings around. :)

(In reply to Chris Peterson [:cpeterson] from comment #13)

I don't have Android Studio, so I can't help here. Where does the Android Studio save your local setting? I would be surprised there isn't a way to save Android Studio settings to a file that can be checked into version control.

There is a file that I can export, but it would require manual importing by every other person in order for it to take effect.

This Android doc page talks about disabling lint checks from Gradle. Perhaps that is an option?

Unfortunately this doesn't work for the same reason that lint.xml doesn't work: it is not a lint warning.

I think suppressing the warnings using @SuppressWarnings("BooleanMethodIsAlwaysInverted") annotations is much better than leaving the warnings around. :)

I will go ahead and do that!

I've attached a patch that uses the @SuppressWarnings method of suppressing this bug!