Add ContextUtils.getCurrentPackageInfo

RESOLVED FIXED in Firefox 49

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mcomella, Assigned: Mouaad Aallam, Mentored)

Tracking

unspecified
Firefox 49
All
Android
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Comment 1

2 years ago
Assign this to me please i will work on it :)
(Reporter)

Comment 2

2 years ago
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.
Assignee: nobody → Aallam.Mouaad
(Assignee)

Comment 3

2 years ago
Created attachment 8741554 [details] [diff] [review]
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
Attachment #8741554 - Flags: review?(michael.l.comella)
(Reporter)

Comment 4

2 years ago
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.
Attachment #8741554 - Flags: review?(michael.l.comella) → feedback+
(Assignee)

Comment 5

2 years ago
Created attachment 8746045 [details] [diff] [review]
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?
Attachment #8746045 - Flags: review?(michael.l.comella)
(Assignee)

Comment 6

2 years ago
Created attachment 8746123 [details] [diff] [review]
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
Attachment #8741554 - Attachment is obsolete: true
Attachment #8746045 - Attachment is obsolete: true
Attachment #8746045 - Flags: review?(michael.l.comella)
Attachment #8746123 - Flags: review?(michael.l.comella)
(Assignee)

Updated

2 years ago
Attachment #8746123 - Flags: checkin?
(Reporter)

Comment 7

2 years ago
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.
Attachment #8746123 - Flags: review?(michael.l.comella) → feedback+

Comment 8

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

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ceddbbe98722
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

2 years ago
Attachment #8746123 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.