1407728 - Add "reason" property to messages sent with createNotifyDevicesPayload

Status: RESOLVED FIXED

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

Description

[Edouard Oger [:eoger]]

This is the Android client work of bug 1400925

Comment 1

[Edouard Oger [:eoger]]

Attachment: Bug 1407728 - Add reason field to messages sent with /notify.

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

Comment 2

[:Grisha Kruglov]

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.

Comment 3

[Edouard Oger [:eoger]]

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.

Comment 4

[:Grisha Kruglov]

That makes sense. Probably no sense in sending that push message 20 minutes after the fact.

Comment 5

[Edouard Oger [:eoger]]

Comment on attachment 8917526 [details]
Bug 1407728 - Add reason field to messages sent with /notify.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff0c7c7b1074
Add reason field to messages sent with /notify. r=Grisha

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ff0c7c7b1074