1042383 - Use optimizable build-time flags for SDK-dependent code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

… as opposed to pure run-time checking of SDK_INT. This allows for ProGuard to eliminate unreachable code when producing SDK-dependent APKs.

Comment 1

[Chris Kitching [:ckitching]]

Note that expressions of the form:

if (SOME_CONSTANT_FALSE) {
   ...
}

will be deleted by javac alone: no need for Proguard there.

This may become a useful factoid when considering how much/if to run Proguard on developer builds.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Part 1: define version capabilities in AppConstants. v1

This is the first step: defining these in such a way that they'll largely be constants if built with the right flags.

For instance, if we build an APK that has a minSDK of 13, all of the 11+ and 13+ conditionals *must* be true, or the APK wouldn't have been installed. Similarly, we know the <13 checks must all fail. These simplifications then chain into clauses in part 2, allowing for dead code removal.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Part 2: use optimizable build-time flags for SDK-dependent code. v1

Saving work. This is pretty much done, but I want to test more.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8460627 [details] [diff] [review]
Part 1: define version capabilities in AppConstants. v1

Since MOZ_ANDROID_BEAM is currently enabled, let's make sure it stays enabled after this lands. Kind of a canary for Versions.featureXXPlus.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Part 2: use optimizable build-time flags for SDK-dependent code. v2

Rebased.

Comment 6

[Richard Newman [:rnewman]]

https://tbpl.mozilla.org/?tree=Try&rev=12d19c44507c

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8463670 [details] [diff] [review]
Part 2: use optimizable build-time flags for SDK-dependent code. v2

>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>-        aMenu.findItem(R.id.quit).setVisible(clearItems.size() > 0 || Build.VERSION.SDK_INT < 14 || HardwareUtils.isTelevision());
>+        final boolean visible = Versions.preICS ||
>+                                clearItems.size() > 0 ||
>+                                HardwareUtils.isTelevision();
>+        aMenu.findItem(R.id.quit).setVisible(visible);

Is the local variable needed? I'd rather drop it. And we don't need to add the wrapping...

>-        if (AppConstants.MOZ_ANDROID_BEAM && Build.VERSION.SDK_INT >= 10 && NfcAdapter.ACTION_NDEF_DISCOVERED.equals(action)) {
>+        if (AppConstants.MOZ_ANDROID_BEAM &&
>+            NfcAdapter.ACTION_NDEF_DISCOVERED.equals(action)) {

Drop the wrapping

>diff --git a/mobile/android/base/ContactService.java b/mobile/android/base/ContactService.java

>-        return (Build.VERSION.SDK_INT >= 11 && !cursor.isNull(GROUP_AUTO_ADD) &&
>+        return (Versions.feature11Plus &&
>+                !cursor.isNull(GROUP_AUTO_ADD) &&
>                 cursor.getInt(GROUP_AUTO_ADD) != 0);

This already had wrapping, so I won't ding you for it

>diff --git a/mobile/android/base/animation/AnimatorProxy.java b/mobile/android/base/animation/AnimatorProxy.java

>-        if (proxy == null || (needsAnimationProxy && proxy.mImpl != view.getAnimation())) {
>+        if (proxy == null ||
>+            (needsAnimationProxy && proxy.mImpl != view.getAnimation())) {

BOOOO!

>diff --git a/mobile/android/base/db/PerProfileDatabaseProvider.java b/mobile/android/base/db/PerProfileDatabaseProvider.java
>diff --git a/mobile/android/base/gfx/LayerView.java b/mobile/android/base/gfx/LayerView.java
>diff --git a/mobile/android/base/gfx/OverscrollEdgeEffect.java b/mobile/android/base/gfx/OverscrollEdgeEffect.java

>-    public boolean draw(final EdgeEffect edge, final Canvas canvas, final float translateX, final float translateY, final float rotation) {
>+    private static boolean draw(final EdgeEffect edge, final Canvas canvas, final float translateX, final float translateY, final float rotation) {

These changes can make it hard to rubberstamp

Comment 8

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #7)

> Is the local variable needed? I'd rather drop it. And we don't need to add
> the wrapping...

Without the wrapping it's a 120-char line, which is a slight stretch, even for me.

But I just noticed that we don't need to hit prefs for clearItems on preICS, so I'm going to make that nice optimization here, and make this moot.

Wrapping undone elsewhere.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Richard Newman [:rnewman] from comment #8)

> But I just noticed that we don't need to hit prefs for clearItems on preICS,
> so I'm going to make that nice optimization here, and make this moot.

Pre-coffee, this is wrong. Never mind.

Comment 10

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/d4dc5916f039
https://hg.mozilla.org/integration/fx-team/rev/5a5018a72c8d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4dc5916f039
https://hg.mozilla.org/mozilla-central/rev/5a5018a72c8d