Closed Bug 1407728 Opened 4 years ago Closed 4 years ago

Add "reason" property to messages sent with createNotifyDevicesPayload

Categories

(Firefox for Android Graveyard :: Android Sync, enhancement)

enhancement
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(1 file)

This is the Android client work of bug 1400925
Comment on attachment 8917526 [details]
Bug 1407728 - Add reason field to messages sent with /notify.

https://reviewboard.mozilla.org/r/188480/#review194140

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:199
(Diff revision 1)
>          return;
>        }
>        checkAndUpload();
>        if (isFirstLocalClientRecordUpload && account != null) {
>          final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
> -        notifyAllClients(fxAccount);
> +        notifyAllClients(fxAccount, 0, COLLECTION_MODIFIED_REASON_FIRSTSYNC);

You've changed this behaviour, so I will ask why.

TTL of 0 usually in context of google's push messages means "try to deliver once" (iirc). What does it mean for APNS or for webpush?

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/stage/SyncClientsEngineStage.java:250
(Diff revision 1)
>      }
>  
>      @NonNull
>      @SuppressWarnings("unchecked")
> -    private ExtendedJSONObject createNotifyClientsBody(@NonNull List<String> devicesToNotify) {
> +    private ExtendedJSONObject createNotifyClientsBody(@NonNull List<String> devicesToNotify,
> +                                                       long TTL, @NonNull String reason) {

nit: here and elsewhere, prefer `ttl`, I think.
Attachment #8917526 - Flags: review?(gkruglov) → review+
Comment on attachment 8917526 [details]
Bug 1407728 - Add reason field to messages sent with /notify.

https://reviewboard.mozilla.org/r/188480/#review194140

> You've changed this behaviour, so I will ask why.
> 
> TTL of 0 usually in context of google's push messages means "try to deliver once" (iirc). What does it mean for APNS or for webpush?

Ha my bad, I've been meaning to explain that change.
I noticed that we used the 1h TTL for both messages, which is wrong.
collection_changed for firstsync should be 0, just like the Desktop and iOS implementation.
That makes sense. Probably no sense in sending that push message 20 minutes after the fact.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff0c7c7b1074
Add reason field to messages sent with /notify. r=Grisha
Product: Android Background Services → Firefox for Android
https://hg.mozilla.org/mozilla-central/rev/ff0c7c7b1074
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.