1187382 - Revise AppConstants.Versions after release of Android M

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

In bug 1183559 I introduced AppConstants.Versions.preM to disable a feature for devices running Android M and above.

After the release of Android M we should:
* Rename preM to the actual release name of Android M
* Validate that Android M is actually API level 23
* Maybe remove the Build.VERSION.RELEASE check for Android M preview releases

Comment 1

[Giovanny Gongora [:gioyik]]

Hi Sebastian,

I would like to work on this, but seems Android M needs to be released first. So, could I get assigned to this bug and I will back after Android M launch?

Thanks

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Hi Giovanny! You are right. We will have to wait until Android M is released. Feel free to attach a patch to this bug as soon as it has been released and I'll assign the bug to you. I'll try to ping you here if I see anything being released. :)

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Hey Giovanny. Android M Preview 3 (Marshmallow) is out and it's API level 23. :)

Comment 4

[Giovanny Gongora [:gioyik]]

Attachment: 1187382.patch

Comment 5

[Giovanny Gongora [:gioyik]]

Hi Sebastian,

Thanks for the alert, I attached a patch with the changes. Let me know if it is OK.

Curious but I was convinced since the beginning that Marshmallow will going to be the name for this Android release. :)

Marshmallows for all the Mozilla Android/Mobile team \o/

http://goo.gl/iAMEui

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8651348 [details] [diff] [review]
1187382.patch

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

LGTM. Thank you Giovanny. :)

::: mobile/android/base/AppConstants.java.in
@@ +64,5 @@
>           * If our MAX_SDK_VERSION is lower than ICS, we must not be an ICS device.
>           * Otherwise, we need a range check.
>           */
> +	public static final boolean preMarshmallow = MAX_SDK_VERSION < 23 ||
> +                (MIN_SDK_VERSION < 23 && Build.VERSION.SDK_INT < 23);

Let's remove the line break too.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Giovanny: Do you have time to push this over the finish line? :)

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Maybe someone else wants to pick it up from here.

Comment 9

[friedger]

Attachment: removed the line break (friedger2.patch)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8670098 [details] [diff] [review]
removed the line break (friedger2.patch)

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

Thanks! Can you create a single, combined patch? :)

::: mobile/android/base/AppConstants.java.in
@@ +63,5 @@
>           * If our MIN_SDK_VERSION is 14 or higher, we must be an ICS device.
>           * If our MAX_SDK_VERSION is lower than ICS, we must not be an ICS device.
>           * Otherwise, we need a range check.
>           */
> +	    public static final boolean preM = MAX_SDK_VERSION < 23 || (MIN_SDK_VERSION < 23 && Build.VERSION.SDK_INT < 23);

It looks like there's a mix of tabs and regular whitespaces at the beginning of this line. Just use four spaces for indentation. :)

Comment 11

[Mouaad Aallam]

Hello :)
I'am new here ! I was looking for some first simple bugs to start, I don't get what is left to do about this bug ?

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mouaad from comment #11)
> Hello :)
> I'am new here ! I was looking for some first simple bugs to start, I don't
> get what is left to do about this bug ?

Hi Mouaad! Basically just combining the two patches. Or rewriting the patch because the code could have changed in the meantime.

Comment 13

[u563752]

(In reply to Sebastian Kaspari (:sebastian) from comment #12)

> Hi Mouaad! Basically just combining the two patches. Or rewriting the patch
> because the code could have changed in the meantime.

Hello :) !  Alright then, can i give it a shot and try do it ? I succeeded to compile the project and run it on my device.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mouaad from comment #13)
> Hello :) !  Alright then, can i give it a shot and try do it ? I succeeded
> to compile the project and run it on my device.

Yeah, go ahead. Let me know if you need any help.

Comment 15

[juzerkhambaty]

Is this still open to fix? I was thinking of picking this up. I figure that 4 files might need changing. I have a question though, the AppConstants.java file gets regenerated every time I run and hence, changes 'PreMarshmallow' back to 'PreM'. Anything I can do about this?

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to juzerkhambaty from comment #15)
> Is this still open to fix? I was thinking of picking this up. I figure that
> 4 files might need changing. I have a question though, the AppConstants.java
> file gets regenerated every time I run and hence, changes 'PreMarshmallow'
> back to 'PreM'. Anything I can do about this?

Yeah, this is still open. You'll need to edit AppConstants.java.in - this is the template to generate AppConstants.java:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AppConstants.java.in

(Feel free to use the need-info flag so that your question will be answered)

Comment 17

[cijo]

Attachment: Bug_1187382 -_Combined two patches into the latest sources

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Thanks for the patch! I'll review it later today.

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8727164 [details] [diff] [review]
Bug_1187382 -_Combined two patches into the latest sources

I have problems applying this patch to the latest tree:

> applying review
> unable to find 'android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java' for patching
> 2 out of 2 hunks FAILED -- saving rejects to file android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java.rej
> unable to find 'android/base/java/org/mozilla/gecko/tabqueue/TabQueueService.java' for patching
> 1 out of 1 hunks FAILED -- saving rejects to file android/base/java/org/mozilla/gecko/tabqueue/TabQueueService.java.rej
> unable to find 'android/base/AppConstants.java.in' for patching
> 1 out of 1 hunks FAILED -- saving rejects to file android/base/AppConstants.java.in.rej
> unable to find 'android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java' for patching
> 1 out of 1 hunks FAILED -- saving rejects to file android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh review

Did you create this patch from IntelliJ? It contains those additional lines for IntelliJ and this might be a problem for mercurial (Also there's no commit message in the patch). Try to create the patch on the command line (hg export).

Comment 20

[cijo]

Attachment: Updated patch

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8727346 [details] [diff] [review]
Updated patch

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

This is looking good! I'll push the patch to try.

::: mobile/android/base/AppConstants.java.in
@@ +62,5 @@
>           * If our MIN_SDK_VERSION is 14 or higher, we must be an ICS device.
>           * If our MAX_SDK_VERSION is lower than ICS, we must not be an ICS device.
>           * Otherwise, we need a range check.
>           */
> +        public static final boolean preMarshmallow = MAX_SDK_VERSION < 23 || (MIN_SDK_VERSION < 23 && Build.VERSION.SDK_INT < 23 && !Build.VERSION.RELEASE.equals("M"));

You can remove !Build.VERSION.RELEASE.equals("M") from here. This was used to exclude M preview builds. No one should be using those anymore.

Comment 22

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=83741ed33b23

Comment 23

[cijo]

Attachment: !Build.VERSION.RELEASE.equals("M") removed

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8727355 [details] [diff] [review]
!Build.VERSION.RELEASE.equals("M") removed

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

LGTM.

nit: Typo "approperiately" -> "appropriately"

Comment 25

[cijo]

Comment on attachment 8727355 [details] [diff] [review]
!Build.VERSION.RELEASE.equals("M") removed

># HG changeset patch
># User cijo <[e-mail]>
># Parent  b6acf4d4fc20431a8ae14bf32cdc6e43a9c0f9ad
>Bug 1187382 - Revise AppConstants.Versions after release of Android M. The assignment logic has also been changed approperiately. -r=sebastian
>
>diff --git a/mobile/android/base/AppConstants.java.in b/mobile/android/base/AppConstants.java.in
>--- a/mobile/android/base/AppConstants.java.in
>+++ b/mobile/android/base/AppConstants.java.in
>@@ -58,18 +58,17 @@ public class AppConstants {
>         public static final boolean feature20Plus = MIN_SDK_VERSION >= 20 || (MAX_SDK_VERSION >= 20 && Build.VERSION.SDK_INT >= 20);
>         public static final boolean feature21Plus = MIN_SDK_VERSION >= 21 || (MAX_SDK_VERSION >= 21 && Build.VERSION.SDK_INT >= 21);
> 
>         /*
>          * If our MIN_SDK_VERSION is 14 or higher, we must be an ICS device.
>          * If our MAX_SDK_VERSION is lower than ICS, we must not be an ICS device.
>          * Otherwise, we need a range check.
>          */
>-	public static final boolean preM = MAX_SDK_VERSION < 23 ||
>-                (MIN_SDK_VERSION < 23 && Build.VERSION.SDK_INT < 23 && !Build.VERSION.RELEASE.equals("M"));
>+        public static final boolean preMarshmallow = MAX_SDK_VERSION < 23 || (MIN_SDK_VERSION < 23 && Build.VERSION.SDK_INT < 23);
>         public static final boolean preLollipop = MAX_SDK_VERSION < 21 || (MIN_SDK_VERSION < 21 && Build.VERSION.SDK_INT < 21);
>         public static final boolean preJBMR2 = MAX_SDK_VERSION < 18 || (MIN_SDK_VERSION < 18 && Build.VERSION.SDK_INT < 18);
>         public static final boolean preJBMR1 = MAX_SDK_VERSION < 17 || (MIN_SDK_VERSION < 17 && Build.VERSION.SDK_INT < 17);
>         public static final boolean preJB = MAX_SDK_VERSION < 16 || (MIN_SDK_VERSION < 16 && Build.VERSION.SDK_INT < 16);
>         public static final boolean preICS = MAX_SDK_VERSION < 14 || (MIN_SDK_VERSION < 14 && Build.VERSION.SDK_INT < 14);
>         public static final boolean preHCMR2 = MAX_SDK_VERSION < 13 || (MIN_SDK_VERSION < 13 && Build.VERSION.SDK_INT < 13);
>         public static final boolean preHCMR1 = MAX_SDK_VERSION < 12 || (MIN_SDK_VERSION < 12 && Build.VERSION.SDK_INT < 12);
>         public static final boolean preHC = MAX_SDK_VERSION < 11 || (MIN_SDK_VERSION < 11 && Build.VERSION.SDK_INT < 11);
>diff --git a/mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java b/mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java
>--- a/mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java
>+++ b/mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImportPreference.java
>@@ -23,17 +23,17 @@ class AndroidImportPreference extends Mu
>     private static final String LOGTAG = "AndroidImport";
>     public static final String PREF_KEY = "android.not_a_preference.import_android";
>     private static final String PREF_KEY_PREFIX = "import_android.data.";
>     private final Context mContext;
> 
>     public static class Handler implements GeckoPreferences.PrefHandler {
>         public boolean setupPref(Context context, Preference pref) {
>             // Feature disabled on devices running Android M+ (Bug 1183559)
>-            return Versions.preM && Restrictions.isAllowed(context, Restrictable.IMPORT_SETTINGS);
>+            return Versions.preMarshmallow && Restrictions.isAllowed(context, Restrictable.IMPORT_SETTINGS);
>         }
> 
>         public void onChange(Context context, Preference pref, Object newValue) { }
>     }
> 
>     public AndroidImportPreference(Context context, AttributeSet attrs) {
>         super(context, attrs);
>         mContext = context;
>diff --git a/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java b/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java
>--- a/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java
>+++ b/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueHelper.java
>@@ -9,25 +9,22 @@ import org.mozilla.gecko.AppConstants;
> import org.mozilla.gecko.GeckoAppShell;
> import org.mozilla.gecko.GeckoEvent;
> import org.mozilla.gecko.GeckoProfile;
> import org.mozilla.gecko.GeckoSharedPrefs;
> import org.mozilla.gecko.R;
> import org.mozilla.gecko.preferences.GeckoPreferences;
> import org.mozilla.gecko.util.ThreadUtils;
> 
>-import android.annotation.TargetApi;
>-import android.app.Notification;
> import android.app.NotificationManager;
> import android.app.PendingIntent;
> import android.content.Context;
> import android.content.Intent;
> import android.content.SharedPreferences;
> import android.content.res.Resources;
>-import android.os.Build;
> import android.provider.Settings;
> import android.support.v4.app.NotificationCompat;
> import android.support.v4.content.ContextCompat;
> import android.text.TextUtils;
> import android.util.Log;
> import org.json.JSONArray;
> import org.json.JSONException;
> import org.json.JSONObject;
>@@ -59,17 +56,17 @@ public class TabQueueHelper {
>     /**
>      * Checks if the specified context can draw on top of other apps. As of API level 23, an app
>      * cannot draw on top of other apps unless it declares the SYSTEM_ALERT_WINDOW permission in
>      * its manifest, AND the user specifically grants the app this capability.
>      *
>      * @return true if the specified context can draw on top of other apps, false otherwise.
>      */
>     public static boolean canDrawOverlays(Context context) {
>-        if (AppConstants.Versions.preM) {
>+        if (AppConstants.Versions.preMarshmallow) {
>             return true; // We got the permission at install time.
>         }
> 
>         return Settings.canDrawOverlays(context);
>     }
> 
>     /**
>      * Check if we should show the tab queue prompt
>diff --git a/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueService.java b/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueService.java
>--- a/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueService.java
>+++ b/mobile/android/base/java/org/mozilla/gecko/tabqueue/TabQueueService.java
>@@ -228,17 +228,17 @@ public class TabQueueService extends Ser
>                 Telemetry.addToHistogram("FENNEC_TABQUEUE_QUEUESIZE", queuedTabCount);
>             }
>         });
> 
>     }
> 
>     @TargetApi(Build.VERSION_CODES.M)
>     private void showSettingsNotification() {
>-        if (AppConstants.Versions.preM) {
>+        if (AppConstants.Versions.preMarshmallow) {
>             return;
>         }
> 
>         final Intent intent = new Intent(Settings.ACTION_MANAGE_OVERLAY_PERMISSION);
>         intent.setData(Uri.parse("package:" + getPackageName()));
>         PendingIntent pendingIntent = PendingIntent.getActivity(this, intent.hashCode(), intent, 0);
> 
>         final String text = getString(R.string.tab_queue_notification_settings);

Comment 26

[cijo]

Attachment: final patch

Comment 27

[Pulsebot]

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

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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