1261040 - Remove code guarded by AppConstants.Versions.preICS

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

ICS is our base level and code for lower platform versions shouldn't be needed anymore:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AppConstants.java.in#71

Comment 1

[Jorick Caberio]

I would like to take on this bug.

Comment 2

[Jorick Caberio]

Attachment: mysecondpatch.diff

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8738937 [details] [diff] [review]
mysecondpatch.diff

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

Great start! It seems like we can remove even more stuff. And let's remove the constant Versions.preICS too.

::: mobile/android/base/java/org/mozilla/gecko/DoorHangerPopup.java
@@ -325,5 @@
>          // causes crashes, so it is done after the popup is shown.
>          if (Versions.feature14Plus) {
>              setFocusable(true);
>          }
> -

NIT: Let's keep the empty line.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ -1400,5 @@
> -
> -        if (mCameraView == null) {
> -            // Pre-ICS devices need the camera surface in a visible layout.
> -            if (Versions.preICS) {
> -                mCameraView = new SurfaceView(this);

It looks like this camera view is only needed for Pre-ICS devices. We might be able to remove it completely - Can you try that?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
@@ -1462,5 @@
> -            if (Versions.preICS) {
> -                prefSetter = new CheckBoxPrefSetter();
> -            } else {
> -                prefSetter = new TwoStatePrefSetter();
> -            }

It looks like this was needed to support setting those checkbox peferences in a backwards compatible way. We should be able to remove CheckBoxPrefSetter and TwoStatePrefSetter. The code that used prefSetter can be replaced with what TwoStatePrefSetter does.

Comment 4

[Jorick Caberio]

(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> Comment on attachment 8738937 [details] [diff] [review]
> mysecondpatch.diff
> 
> Review of attachment 8738937 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great start! It seems like we can remove even more stuff. And let's remove
> the constant Versions.preICS too.
> 
> ::: mobile/android/base/java/org/mozilla/gecko/DoorHangerPopup.java
> @@ -325,5 @@
> >          // causes crashes, so it is done after the popup is shown.
> >          if (Versions.feature14Plus) {
> >              setFocusable(true);
> >          }
> > -
> 
> NIT: Let's keep the empty line.
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
> @@ -1400,5 @@
> > -
> > -        if (mCameraView == null) {
> > -            // Pre-ICS devices need the camera surface in a visible layout.
> > -            if (Versions.preICS) {
> > -                mCameraView = new SurfaceView(this);
> 
> It looks like this camera view is only needed for Pre-ICS devices. We might
> be able to remove it completely - Can you try that?
> 

Just to clarify, I'll delete the mCameraView variable and it's usages in GeckoApp.java


> :::
> mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
> @@ -1462,5 @@
> > -            if (Versions.preICS) {
> > -                prefSetter = new CheckBoxPrefSetter();
> > -            } else {
> > -                prefSetter = new TwoStatePrefSetter();
> > -            }
> 
> It looks like this was needed to support setting those checkbox peferences
> in a backwards compatible way. We should be able to remove
> CheckBoxPrefSetter and TwoStatePrefSetter. The code that used prefSetter can
> be replaced with what TwoStatePrefSetter does.

Comment 5

[Jorick Caberio]

Attachment: myfourthpatch.diff

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8739388 [details] [diff] [review]
myfourthpatch.diff

Just set the new patch to "review?" and select a reviewer. Or even better: Create a new patch with all changes and replace the previous one. :)

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8739388 [details] [diff] [review]
myfourthpatch.diff

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

This doesn't compile:
> 0:45.97 /Users/sebastian/Projects/Mozilla/fx-team/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:177: error: BrowserApp is not abstract and does not override abstract method getCameraView() in GeckoInterface
> 0:45.97 public class BrowserApp extends GeckoApp

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Before this bug gets too big, maybe let's just handle the flags here and create new follow-up bugs for CameraView and TwoStatePrefSetter.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll create new bugs for the CameraView and TwoStatePrefSetter.

I'll clean up the patch here and push it to try and then eventually land it.

Comment 10

[Jorick Caberio]

Thanks. Please also notify me about the CameraView and TwoStatePrefSetter bugs.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84cb774e4a24

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8738937 [details] [diff] [review]
mysecondpatch.diff

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

LGTM. Passed tests on try.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/4bc053de842538e99e56927b3c03fdc539374a16
Bug 1261040 - Remove code code guarded by AppConstants.Versions.preICS. r=sebastian

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/4bc053de8425