1262275 - Add ContextUtils.getCurrentPackageInfo

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Returns:

context.getPackageManager().getPackageInfo(context.getPackageName(), 0)

and throws an AssertionError (or IllegalStateException) when a NameNotFoundException is thrown, because we'd never expect that to happen. Make other code that gets the current package use this code:

https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/mobile/android/base/java/org/mozilla/gecko/dlc/StudyAction.java#68

and bug 1246816 adds ContextUtils.getPackageInstall time – we can probably replace that with this method.

Comment 1

[Mouaad Aallam]

Assign this to me please i will work on it :)

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sure Mouaad. Let us know if you need any help!

The dependency landed only 7 days ago so make sure you have a recent pull of the code.

Comment 3

[Mouaad Aallam]

Attachment: Patch for bug 1262275

Hello, Michael ! :D
Here is a first try patch :), not sure about the AssertionError error message if it's the correct one. of course, I built this and run on my device successfully 

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=38fcff7a1fe4

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8741554 [details] [diff] [review]
Patch for bug 1262275

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

Besides removing the existing ContextUtils, this looks good to me.

Thanks for pushing your own tests! :)

::: mobile/android/base/java/org/mozilla/gecko/mozglue/ContextUtils.java
@@ +26,5 @@
>      public static boolean getBooleanExtra(Intent intent, String name, boolean defaultValue) {
>          return new SafeIntent(intent).getBooleanExtra(name, defaultValue);
>      }
>  
> +    public static PackageInfo getCurrentPackageInfo(final Context context){

This should be in the other ContextUtils class (the one you removed) – mozglue is separate from util.

See also bug 1261210.

Comment 5

[Mouaad Aallam]

Attachment: Updating the correct ContextUtils Class

Hello, Michael! :D

Please check this patch ! (Not sure why Android Studio 2.1 reordered the imports in the class GeckoProfile.java though !)

> See also bug 1261210.
Should I integrate a patch for this bug too?

Comment 6

[Mouaad Aallam]

Attachment: Updated Patch

Hello again !
The previous patch had a missing whitespace before an "{".
Please consider this patch :)

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=350f4a135fa0

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8746123 [details] [diff] [review]
Updated Patch

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

Looking pretty good – almost there!

Thanks for your help so far!

::: mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java
@@ +26,5 @@
> +import org.mozilla.gecko.firstrun.FirstrunAnimationContainer;
> +import org.mozilla.gecko.mozglue.ContextUtils;
> +import org.mozilla.gecko.preferences.DistroSharedPrefsImport;
> +import org.mozilla.gecko.util.INIParser;
> +import org.mozilla.gecko.util.INISection;

While I actually don't care about ordering, changing imports unnecessarily does make it harder to uplift/backout code – do you mind putting this back?

I'm not sure why AS did this – perhaps you used an "Organize imports" function?

::: mobile/android/base/java/org/mozilla/gecko/util/ContextUtils.java
@@ +13,5 @@
>  public class ContextUtils {
>      private ContextUtils() {}
>  
>      /**
>       * @return {@link android.content.pm.PackageInfo#firstInstallTime} for the context's package.

nit: can you update the javadoc?

@@ +22,3 @@
>          try {
> +            return context.getPackageManager().getPackageInfo(context.getPackageName(), 0);
> +        } catch (PackageManager.NameNotFoundException e) {

nit: final

On that note, I'm not sure why these catch lines are shown as changed – I don't see any changes. Can you make sure the indentation is correct (maybe you're using tabs instead of spaces?)?

You can use `./mach gradle checkstyle` to check for tabs.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ceddbbe98722

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ceddbbe98722