Eliminate Honeycomb compatibility from Fennec code

RESOLVED FIXED in Firefox 56

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: rnewman, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 56
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 wontfix, firefox45 wontfix, firefox56 fixed)

Details

(Whiteboard: [lang=java][good next bug], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This is stage two of Bug 1155801. That bug changes our build configurations to no longer target Honeycomb in our main APK.

See Bug 1155801 Comment 17:

---
The second stage is that we actively remove code that specifically supports Honeycomb -- workarounds for platform bugs, media code, the use of compat libraries, Honeycomb branches in switch statements.

At that point we will not run correctly on 11-13: those workarounds exist for a reason.

It'll be possible to produce a build that will *install* on Honeycomb, but it's likely to crash or be buggy -- hitting tablet code paths that no longer contain Honeycomb workarounds, for example. 

This will be the first time that we will only support discontinuous API ranges.
---
(Reporter)

Updated

2 years ago
See Also: → bug 1017242
(Reporter)

Updated

2 years ago
Blocks: 1220184
(Reporter)

Comment 1

2 years ago
Maybe a good second bug here for a contributor, Michael and Margaret…
status-firefox44: affected → wontfix
status-firefox45: --- → wontfix
(In reply to Richard Newman [:rnewman] from comment #1)
> Maybe a good second bug here for a contributor, Michael and Margaret…

To be explicit, this bug is about:
* Changing the AppConstants.Versions.feature11Plus & *.feature12Plus branches to *.feature14Plus & removing the constants
* Changing AC.Versions.preHC* branches to AC.Versions.preHC & removing the constants
* Completely eliminating Honeycomb specific branches, such as `if (*.preICS && *.feature11Plus)`

Richard, would you concur?
Mentor: michael.l.comella@gmail.com, margaret.leibovic@gmail.com
Flags: needinfo?(rnewman)
Whiteboard: [lang=java][good next bug]
(Reporter)

Comment 3

2 years ago
Broadly, yes. The Versions fields themselves can stay or go, but should be commented.

Code that uses them can be removed, or replaced by something like `throw new UnsupportedAndroidVersionException()`, which will give us a unique stack if we screw up somehow.

I think the latter is worth doing in significant places; it'll avoid the possibility that someone will fork Fennec, do a build for 9-23, and have mismatched expectations about bugginess with our Honeycomb compat work removed!
Flags: needinfo?(rnewman)
(Reporter)

Updated

2 years ago
Depends on: 1237342
(Reporter)

Comment 4

2 years ago
Note that Bug 1237342 dropped support for API 14, so this bug can kill a little more code.
I'd be interested in working on this one!
(Reporter)

Comment 6

2 years ago
Please do! I encourage you to file sub-bugs for this -- e.g., "Remove Honeycomb compatibility code and introduce Honeycomb warnings from db code" -- blocking this one. That way this doesn't all need to get done in a single release.
Assignee: nobody → me
Status: NEW → ASSIGNED
Hi Alex, is this still something you are working on? Otherwise, let's unassign you and let other people have a chance to pick this up! In general, we also try to assign bugs when there's a patch, and rely on comments to "claim" a bug.
Flags: needinfo?(me)
I have busy with school for the past couple weeks.  I will be busy until mid May.  I will unassign the bug then, so someone else can pick it up.
Assignee: me → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(me)

Comment 9

a year ago
If this is not fixed I want to take a try, please respond if it is possible to work on this :)
(In reply to svezauzeto12 from comment #9)
> If this is not fixed I want to take a try, please respond if it is possible
> to work on this :)

Absolutely!  #c2 and #c4 still apply, modulo that Fennec is API 15+ now.

However, there's a new wrinkle -- Fennec is now split into an App and the GeckoView library.  The code for the App is in mobile/android/base and the code for the GeckoView library is in mobile/android/geckoview.  We want Fennec to be API 15+ but we'd like to keep GeckoView to be whatever it can be -- 9+ if possible.  So start making these changes just for mobile/android/base.

Thanks for helping!

Comment 11

a year ago
Is it possible to get little more detail what to do here, sorry for bothering.

I have found AppConstants.java.in in mobile/android/base but I am not sure what to do from here.

Comment 12

8 months ago
There are no pre15 constants anymore, also minSdkVersion of geckoview in build.gradle is 15 as well.

Looks liked fixed.
Flags: needinfo?(nalexander)
(In reply to friedger from comment #12)
> There are no pre15 constants anymore, also minSdkVersion of geckoview in
> build.gradle is 15 as well.
> 
> Looks liked fixed.

It's mostly fixed, but sadly I see a lot of direct checks against Build.SDK_INT for versions we no longer support in the codebase: https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=.SDK_INT.

I would like to see the AppConstants.Versions code be replaced with simple inline Build.SDK_INT checks because then the standard Android linter will warn us in AS if we do unsafe things.  As of a long time ago, the linter did _not_ warn with our AppConstants.Versions checks, 'cuz it only looks for the simplest patterns.

Thanks for checking in on this!
Flags: needinfo?(nalexander)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

8 months ago
mozreview-review
Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

https://reviewboard.mozilla.org/r/142208/#review145878

::: mobile/android/stumbler/java/org/mozilla/mozstumbler/service/Prefs.java:182
(Diff revision 2)
>      }
>  
>      private void setBoolPref(String key, Boolean state) {
>          SharedPreferences.Editor editor = getPrefs().edit();
>          editor.putBoolean(key,state);
> -        apply(editor);
> +        editor.apply();

This could be inlined, would that be part of this bug?

Comment 17

8 months ago
mozreview-review
Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

https://reviewboard.mozilla.org/r/142208/#review146104

This is a great first cut!  r- just to scope down the patch:

* We don't own `media/webrtc/trunk/webrtc/`, so don't touch that code.  It's imported from a shared upstream repository, IIRC.

* We also don't really own `mobile/android/thirdparty`, so let's not modify that either.

Otherwise, I just have some small issues.  Thanks!

::: mobile/android/base/java/org/mozilla/gecko/Tab.java:251
(Diff revision 2)
>          mMostRecentHomePanelData = data;
>      }
>  
>      public Bitmap getThumbnailBitmap(int width, int height) {
>          if (mThumbnailBitmap != null) {
>              // Bug 787318 - Honeycomb has a bug with bitmap caching, we can't

Kill the comment too.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
(Diff revision 2)
>  
> -            if (Build.VERSION.SDK_INT < 13) {
> -                // Affected by Bug 1015209 -- no detach/attach.
> -                // If we try rejigging fragments, we'll crash, so don't
> -                // enable locale switching at all.
> -                localeSwitchingIsEnabled = false;

This flag is no longer required, since there's no purely local override.  File follow-up (a mentor ticket?) to simplify this logic.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java:583
(Diff revision 2)
>  
>                  id = insertWithNullCol(BrowserContract.Bookmarks.TYPE);
>                  mAsserter.is(id, -1L,
>                               "Should not be able to insert bookmark with null type");
>  
> -                if (Build.VERSION.SDK_INT >= 8 &&
> +                if (Build.VERSION.SDK_INT == 15) {

Prefer `< 16`, here and below.  Just a little safer; doesn't bake in that our minimum is 15 in quite the same way.

::: mobile/android/thirdparty/com/leanplum/LeanplumActivityHelper.java:98
(Diff revision 2)
>    public static boolean isActivityPaused() {
>      return isActivityPaused;
>    }
>  
>    /**
>     * Enables lifecycle callbacks for Android devices with Android OS &gt;= 4.0

I think you can kill the comment, since the content is in the method name.
Attachment #8870727 - Flags: review?(nalexander) → review-
Comment hidden (mozreview-request)

Comment 19

8 months ago
mozreview-review-reply
Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

https://reviewboard.mozilla.org/r/142208/#review146104

> This flag is no longer required, since there's no purely local override.  File follow-up (a mentor ticket?) to simplify this logic.

I created this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1369673

> I think you can kill the comment, since the content is in the method name.

This is thirdparty

Updated

8 months ago
Blocks: 1369673

Comment 20

8 months ago
mozreview-review
Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

https://reviewboard.mozilla.org/r/142208/#review149252

If it's green on try, it looks good!  Thanks for picking this up!

::: mobile/android/thirdparty/com/leanplum/Leanplum.java:1024
(Diff revision 3)
>  
>    /**
>     * Call this to explicitly end the session. This should not be used in most cases, so we won't
>     * make it public for now.
>     */
> -  static void stop() {
> +  public static void stop() {

nit: do these _need_ to change?  I doubt it.
Attachment #8870727 - Flags: review?(nalexander) → review+
Comment hidden (mozreview-request)

Updated

8 months ago
Attachment #8875641 - Flags: review?(nalexander)

Comment 22

8 months ago
mozreview-review
Comment on attachment 8875641 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

https://reviewboard.mozilla.org/r/147066/#review151382

Great!  Fold this into the previous commit and bombs away.
Attachment #8875641 - Flags: review?(nalexander) → review+
Comment hidden (mozreview-request)

Updated

8 months ago
Attachment #8875641 - Attachment is obsolete: true

Comment 24

8 months ago
I tried to squash the commits, however, not sure whether this is not correct
Flags: needinfo?(nalexander)
(In reply to friedger from comment #24)
> I tried to squash the commits, however, not sure whether this is not correct

Looks good.  I will trigger landing!
Flags: needinfo?(nalexander)

Comment 26

7 months ago
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42670967d5ca
Eliminate Honeycomb compatibility from Fennec code r=nalexander

Comment 27

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42670967d5ca
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.