Closed Bug 1042383 Opened 7 years ago Closed 7 years ago

Use optimizable build-time flags for SDK-dependent code

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(2 files, 1 obsolete file)

… as opposed to pure run-time checking of SDK_INT. This allows for ProGuard to eliminate unreachable code when producing SDK-dependent APKs.
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.
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.
Attachment #8460627 - Flags: review?(mark.finkle)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Saving work. This is pretty much done, but I want to test more.
Component: Build Config & IDE Support → General
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.
Attachment #8460627 - Flags: review?(mark.finkle) → review+
Attachment #8460628 - Attachment is obsolete: true
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
Attachment #8463670 - Flags: review?(mark.finkle) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/d4dc5916f039
https://hg.mozilla.org/mozilla-central/rev/5a5018a72c8d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Blocks: 1048421
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.