1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Comment 1

[Andrei Lazar]

Attachment: Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

Refactored existing LeanplumPushInstanceIDService to support Oreo background
execution limits in Leanplum after upgrade.

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

Comment 2

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8989940 [details]
Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

https://reviewboard.mozilla.org/r/254946/#review261906

I have some comments and a thing that needs to be fixed.  I'm generally very concerned that we're heavily modifying "thirdparty" code, and not just by updating our upstream revision.  (If  this _is_ updating from an upstream revision, the commit comment needs to give a clear link to the new version, etc.)

::: mobile/android/base/MmaAndroidManifest_services.xml.in:15
(Diff revision 1)
>              android:enabled="true" />
>  
> +        <service android:name="com.leanplum.LeanplumPushListenerService" android:exported="false">
> +            <intent-filter>
> +                 <action android:name="com.google.android.c2dm.intent.RECEIVE" />
> +                 <category android:name="org.mozilla.fennec_andreilazar" />

This is clearly not okay.

::: mobile/android/thirdparty/com/leanplum/LeanplumGcmRegistrationJobService.java:9
(Diff revision 1)
> +import android.app.job.JobService;
> +
> +/**
> + * Leanplum GCM registration Job Service to start registration service.
> + *
> + * @author Anna Orlova

We never include authorship details.

::: mobile/android/thirdparty/com/leanplum/LeanplumPushInstanceIDService.java:43
(Diff revision 1)
>     * been compromised. This call is initiated by the InstanceID provider.
>     */
>    @Override
>    public void onTokenRefresh() {
> -    Log.i("GCM InstanceID token needs an update");
> -    // Fetch updated Instance ID token and notify our app's server of any changes (if applicable).
> +    try {
> +      if (Build.VERSION.SDK_INT < 26) {

Why 26?  This is all conditional on 21+, isn't it?

Comment 3

[Andrei Lazar]

Comment on attachment 8989940 [details]
Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

https://reviewboard.mozilla.org/r/254946/#review261906

> This is clearly not okay.

Thankies, I've missed this somehow.

> We never include authorship details.

This patch brings to our thirdparty module a change from Leanplum's upstream SDK. This change was made by them, whom also included the authorship in the file. You can check it here: https://github.com/Leanplum/Leanplum-Android-SDK/blob/4ff48e334f4353d7915be78587fc66127742873e/AndroidSDKGcm/src/main/java/com/leanplum/LeanplumGcmRegistrationJobService.java#L31

> Why 26?  This is all conditional on 21+, isn't it?

Same for this: https://github.com/Leanplum/Leanplum-Android-SDK/blob/4ff48e334f4353d7915be78587fc66127742873e/AndroidSDKGcm/src/main/java/com/leanplum/LeanplumPushInstanceIDService.java#L43

Comment 4

[Andrei Lazar]

Comment on attachment 8989940 [details]
Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

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

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8989940 [details]
Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

https://reviewboard.mozilla.org/r/254946/#review262586

This looks fine to me.  I can't verify that the upstream changes are sensible.

::: commit-message-fd9ba:3
(Diff revisions 1 - 2)
>  Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade] r?sdaswani
>  
>  Refactored existing LeanplumPushInstanceIDService to support Oreo background

This commit message needs to include the details of where this thirdparty code is coming from.  Is this copying _all_ of the files from a certain revision upstream?  Just some of the files?  Just some of the change hunks?  (Gosh, I hope it's not just cherry-picked hunks.)

::: mobile/android/base/MmaAndroidManifest_services.xml.in:20
(Diff revision 2)
> +                 <category android:name="@ANDROID_PACKAGE_NAME@" />
> +            </intent-filter>
> +        </service>
> +
> +         <!-- Leanplum GCM Registration Job Service. -->
> +         <service android:name="com.leanplum.LeanplumGcmRegistrationJobService"

Please explicitly `android:exported="true"` here.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1473518 - Abide by Android Oreo background execution limits [Leanplum after upgrade]

Refactored existing LeanplumPushInstanceIDService to support Oreo background
execution limits in Leanplum after upgrade.

***

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4bb9aa2e34
Abide by Android Oreo background execution limits [Leanplum after upgrade] r=nalexander

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2d4bb9aa2e34