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)
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: sebastian, Assigned: jorick.caberio, Mentored)
References
Details
Attachments
(2 files)
10.41 KB,
patch
|
sebastian
:
review+
sebastian
:
feedback+
|
Details | Diff | Splinter Review |
6.65 KB,
patch
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
I would like to take on this bug.
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8738937 -
Flags: review?(s.kaspari)
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → jorick.caberio
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
Attachment #8739388 -
Flags: review+
Reporter | ||
Comment 6•8 years ago
|
||
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)
Reporter | ||
Comment 7•8 years ago
|
||
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+
Reporter | ||
Comment 8•8 years ago
|
||
Before this bug gets too big, maybe let's just handle the flags here and create new follow-up bugs for CameraView and TwoStatePrefSetter.
Reporter | ||
Updated•8 years ago
|
Attachment #8738937 -
Flags: review?(s.kaspari)
Reporter | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
Thanks. Please also notify me about the CameraView and TwoStatePrefSetter bugs.
Reporter | ||
Comment 11•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84cb774e4a24
Reporter | ||
Comment 12•8 years ago
|
||
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+
Reporter | ||
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/4bc053de842538e99e56927b3c03fdc539374a16 Bug 1261040 - Remove code code guarded by AppConstants.Versions.preICS. r=sebastian
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4bc053de8425
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
•