Closed Bug 1438716 Opened 6 years ago Closed 6 years ago

Upgrade Leanplum SDK

Categories

(Firefox for Android Graveyard :: Metrics, enhancement, P1)

All
Android
enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: sdaswani, Assigned: andrei.a.lazar)

References

Details

(Whiteboard: --do_not_change--[priority:high])

Attachments

(2 files)

Hello!

We are releasing a new version of the Android SDK on Friday, February 16th, and have some important improvements that we want to share.

Below is the full list of improvements being made:

    Splits the existing Android SDK into smaller modules e.g. Core, GCM, FCM, Location, UI editor
    Resolves contention issues between GCM and FCM 
    Reduces total SDK package size — by only including the modules you need, you can optimize the overall weight of the LP SDK 

If your project is configured to automatically pull in the latest version of our SDK from Gradle, then you will need to follow the instructions here to correctly compile your project:

https://www.leanplum.com/docs/android/upgrading-to-android-4

Please reach out directly to support@leanplum.com or your CSM with any questions.

Cheers,
The Leanplum Team 

Mobile Marketing Platform
Integrated. ROI Engine.
 
If you no longer wish to receive these email click here to Unsubscribe
Jean what's the priority on this?
Flags: needinfo?(jcollings)
Would this address fixing the banner issues we're having?
Flags: needinfo?(jcollings)
No clue Jean.
[triage] Potentially critical – is Leanplum.
Rank: 1
Priority: -- → P1
Whiteboard: [Leanplum] [60]
Assignee: nobody → andrei.a.lazar
Andrei can you attach the diff for Fennec with the updated Leanplum SDK here please.
Flags: needinfo?(andrei.a.lazar)
PI request sent.
Blocks: 1438680
Nick, it turns out that this SDK upgrade will require API level 26 - what are the plans to upgrade Fennec to API level 26+?
Flags: needinfo?(nalexander)
Whiteboard: [Leanplum] [60] → [Leanplum] [61]
Depends on: android-o
(In reply to :sdaswani from comment #7)
> Nick, it turns out that this SDK upgrade will require API level 26 - what
> are the plans to upgrade Fennec to API level 26+?

There are none.  There are no people working on Fennec to the best of my knowledge.
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #8)
> (In reply to :sdaswani from comment #7)
> > Nick, it turns out that this SDK upgrade will require API level 26 - what
> > are the plans to upgrade Fennec to API level 26+?
> 
> There are none.  There are no people working on Fennec to the best of my
> knowledge.

Thanks Nick, the Softvision contractors are going to give it a go.
(In reply to :sdaswani from comment #9)
> (In reply to Nick Alexander :nalexander from comment #8)
> > (In reply to :sdaswani from comment #7)
> > > Nick, it turns out that this SDK upgrade will require API level 26 - what
> > > are the plans to upgrade Fennec to API level 26+?
> > 
> > There are none.  There are no people working on Fennec to the best of my
> > knowledge.
> 
> Thanks Nick, the Softvision contractors are going to give it a go.

Very good!  I do not expect there will be significant issues.  There's a tricky thing where the versions in https://searchfox.org/mozilla-central/source/old-configure.in#2076 and https://searchfox.org/mozilla-central/source/build/autoconf/android.m4#99 need to be updated; but after that, I expect those few version bumps to just work in https://searchfox.org/mozilla-central/source/build.gradle#18-20.
(In reply to Nick Alexander :nalexander from comment #10)
> (In reply to :sdaswani from comment #9)
> > (In reply to Nick Alexander :nalexander from comment #8)
> > > (In reply to :sdaswani from comment #7)
> > > > Nick, it turns out that this SDK upgrade will require API level 26 - what
> > > > are the plans to upgrade Fennec to API level 26+?
> > > 
> > > There are none.  There are no people working on Fennec to the best of my
> > > knowledge.
> > 
> > Thanks Nick, the Softvision contractors are going to give it a go.
> 
> Very good!  I do not expect there will be significant issues.  There's a
> tricky thing where the versions in
> https://searchfox.org/mozilla-central/source/old-configure.in#2076 and
> https://searchfox.org/mozilla-central/source/build/autoconf/android.m4#99
> need to be updated; but after that, I expect those few version bumps to just
> work in https://searchfox.org/mozilla-central/source/build.gradle#18-20.

Great, this tip and any more would be much appreciated :) .
Depends on: build-android-o
No longer depends on: android-o
In theory this needs to be done before being able to upgrade the Android support library to v26, although in practice the one compile error I got can be worked around, so it might not be absolutely critical.
Blocks: 1385464
OS: Unspecified → Android
Hardware: Unspecified → All
Andrei now that we've upgraded to Android O / API Level 26, can the Leanplum work move forth?
Attachment #8974442 - Flags: review?(sdaswani) → review?(cnevinchen)
Flags: needinfo?(andrei.a.lazar)
May I know the commit or branch in the Leanplum SDK your patch based on? thanks!
Flags: needinfo?(andrei.a.lazar)
Yes, as I've said in the commit message, the branch is release 3.0.2 (https://github.com/Leanplum/Leanplum-Android-SDK/tree/release/3.0.2). Thank you!
Flags: needinfo?(andrei.a.lazar)
Comment on attachment 8974442 [details]
Bug 1438716 - Upgrade Leanplum SDK.

https://reviewboard.mozilla.org/r/242790/#review249670

If there's no error with try, after fixing per comments this patch will be good for landing.

::: mobile/android/thirdparty/build.gradle:56
(Diff revision 1)
>      }
> +    // Provided dependencies are optional dependencies and will not show up in pom file.
> +    compileOnly('com.google.android.gms:play-services-gcm:[8.3.0,)') {
> +        exclude module: 'support-v4'
> +    }
> +    compileOnly('com.google.firebase:firebase-messaging:[10.0.0,)') {

I guess we don't need this cause we didn't and won't use it? See the comments below

::: mobile/android/thirdparty/build.gradle:59
(Diff revision 1)
> +        exclude module: 'support-v4'
> +    }
> +    compileOnly('com.google.firebase:firebase-messaging:[10.0.0,)') {
> +        exclude module: 'support-v4'
> +    }
> +    compileOnly('com.google.android.gms:play-services-location:[10.0.0,)') {

Same as above

::: mobile/android/thirdparty/com/leanplum/LeanplumFcmProvider.java:38
(Diff revision 1)
> +/**
> + * Leanplum provider for work with Firebase.
> + *
> + * @author Anna Orlova
> + */
> +class LeanplumFcmProvider extends LeanplumCloudMessagingProvider {

Since we didn't use it, can we skip and don't add this file?

::: mobile/android/thirdparty/com/leanplum/LeanplumInbox.java:107
(Diff revision 1)
> +  /**
> +   * Returns the identifiers of all inbox messages on the device sorted in ascending
> +   * chronological order, i.e. the id of the oldest message is the first one, and the most recent
> +   * one is the last one in the array.
> +   */
> +  public List<String> messagesIds() {

I'm not sure if it still the case, I think we don't use Inbox. Should we update it as well?

::: mobile/android/thirdparty/com/leanplum/internal/ActionManager.java:80
(Diff revision 1)
>  
>    private static boolean loggedLocationManagerFailure = false;
>  
>    public static LocationManager getLocationManager() {
>      if (Util.hasPlayServices()) {
> +      try {

We returned null on purpose cause we don't want google location service in our codebase.
Please feel free to request for review again. Thanks!
Flags: needinfo?(andrei.a.lazar)
Attachment #8974442 - Flags: review?(cnevinchen)
Hello Nevin, I think this is good to land as soon as we finish the Oreo migration. Thank you!
Flags: needinfo?(andrei.a.lazar)
Blocks: 1399470
Whiteboard: [Leanplum] [61] → --do_not_change--[priority:high]
Comment on attachment 8974442 [details]
Bug 1438716 - Upgrade Leanplum SDK.

https://reviewboard.mozilla.org/r/242790/#review255698

Sorry for the late. Please drop me a message on Slack if you need a prompt reply :)

The fix here seems all from Leanplum. And I didn't see any Firebase or other undesired dependencies so I think it's good to land.
Attachment #8974442 - Flags: review?(cnevinchen) → review+
Status: NEW → ASSIGNED
Blocks: 1463376
Blocks: 1473518
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d686b90e9c64
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: