Closed
Bug 1042383
Opened 9 years ago
Closed 9 years ago
Use optimizable build-time flags for SDK-dependent code
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 34
People
(Reporter: rnewman, Assigned: rnewman)
References
Details
Attachments
(2 files, 1 obsolete file)
5.08 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
167.66 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
… 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•9 years ago
|
||
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.
Assignee | ||
Comment 2•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Saving work. This is pretty much done, but I want to test more.
Assignee | ||
Updated•9 years ago
|
Component: Build Config & IDE Support → General
Comment 4•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Attachment #8460628 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=12d19c44507c
Comment 7•9 years ago
|
||
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+
Assignee | ||
Comment 8•9 years ago
|
||
(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.
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/d4dc5916f039 https://hg.mozilla.org/integration/fx-team/rev/5a5018a72c8d
Comment 11•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d4dc5916f039 https://hg.mozilla.org/mozilla-central/rev/5a5018a72c8d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•