Closed Bug 1473518 Opened Last year Closed Last year

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

Categories

(Firefox for Android :: General, enhancement)

Unspecified
Android
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

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

References

Details

Attachments

(2 files)

No description provided.
Attachment #8989940 - Flags: review?(sdaswani) → review?(nchen)
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 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
Attachment #8989940 - Flags: review?(sdaswani)
Attachment #8989940 - Flags: review?(nalexander)
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.
Attachment #8989940 - Flags: review?(nalexander) → review+
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4bb9aa2e34
Abide by Android Oreo background execution limits [Leanplum after upgrade] r=nalexander
https://hg.mozilla.org/mozilla-central/rev/2d4bb9aa2e34
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Attachment #8992033 - Flags: review?(sdaswani) → review?(nchen)
You need to log in before you can comment on or make changes to this bug.