1217675 - Eliminate Honeycomb compatibility from Fennec code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

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.
---

Comment 1

[Richard Newman [:rnewman]]

Maybe a good second bug here for a contributor, Michael and Margaret…

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(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?

Comment 3

[Richard Newman [:rnewman]]

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!

Comment 4

[Richard Newman [:rnewman]]

Note that Bug 1237342 dropped support for API 14, so this bug can kill a little more code.

Comment 5

[Alex Johnson (:alex_johnson)]

I'd be interested in working on this one!

Comment 6

[Richard Newman [:rnewman]]

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.

Comment 7

[Chenxia Liu [:liuche]]

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.

Comment 8

[Alex Johnson (:alex_johnson)]

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.

Comment 9

[Tomislav Jurin]

If this is not fixed I want to take a try, please respond if it is possible to work on this :)

Comment 10

[Nick Alexander :nalexander [he/him]]

(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

[Tomislav Jurin]

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

[friedger]

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

Looks liked fixed.

Comment 13

[Nick Alexander :nalexander [he/him]]

(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!

Comment 14

[friedger]

Attachment: Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

Remove all references to Build.SDK_INT comparing 14 and lower

Review commit: https://reviewboard.mozilla.org/r/142208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142208/

Comment 15

[friedger]

Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142208/diff/1-2/

Comment 16

[friedger]

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

[Nick Alexander :nalexander [he/him]]

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.

Comment 18

[friedger]

Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142208/diff/2-3/

Comment 19

[friedger]

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

Comment 20

[Nick Alexander :nalexander [he/him]]

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.

Comment 21

[friedger]

Attachment: Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

Remove all references to Build.SDK_INT comparing 14 and lower

Review commit: https://reviewboard.mozilla.org/r/147066/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/147066/

Comment 22

[Nick Alexander :nalexander [he/him]]

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.

Comment 23

[friedger]

Comment on attachment 8870727 [details]
Bug 1217675 - Eliminate Honeycomb compatibility from Fennec code

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142208/diff/3-4/

Comment 24

[friedger]

I tried to squash the commits, however, not sure whether this is not correct

Comment 25

[Nick Alexander :nalexander [he/him]]

(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!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42670967d5ca
Eliminate Honeycomb compatibility from Fennec code r=nalexander

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/42670967d5ca