Closed Bug 1261040 Opened 8 years ago Closed 8 years ago

Remove code guarded by AppConstants.Versions.preICS

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: jorick.caberio, Mentored)

References

Details

Attachments

(2 files)

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
I would like to take on this bug.
Attachment #8738937 - Flags: review?(s.kaspari)
Assignee: nobody → jorick.caberio
Status: NEW → ASSIGNED
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.
Attachment #8738937 - Flags: review?(s.kaspari) → feedback+
(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.
Attachment #8739388 - Flags: review+
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. :)
Attachment #8739388 - Flags: review?(s.kaspari)
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
Attachment #8739388 - Flags: review?(s.kaspari)
Attachment #8739388 - Flags: review+
Before this bug gets too big, maybe let's just handle the flags here and create new follow-up bugs for CameraView and TwoStatePrefSetter.
Attachment #8738937 - Flags: review?(s.kaspari)
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.
Thanks. Please also notify me about the CameraView and TwoStatePrefSetter bugs.
Depends on: 1264596
Depends on: 1264599
Comment on attachment 8738937 [details] [diff] [review]
mysecondpatch.diff

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

LGTM. Passed tests on try.
Attachment #8738937 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/integration/fx-team/rev/4bc053de842538e99e56927b3c03fdc539374a16
Bug 1261040 - Remove code code guarded by AppConstants.Versions.preICS. r=sebastian
https://hg.mozilla.org/mozilla-central/rev/4bc053de8425
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.