Closed Bug 1407726 Opened 2 years ago Closed 2 years ago

Add "reason" property to messages sent with _notifyCollectionChanged

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 58
Tracking Status
firefox58 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(1 file)

This is the Desktop client work of bug 1400925
Comment on attachment 8917510 [details]
Bug 1407726 - Add reason field to push messages sent with /notify.

https://reviewboard.mozilla.org/r/188462/#review193720

Looks good to me.

::: services/sync/modules/constants.js:95
(Diff revision 1)
>  MAX_HISTORY_DOWNLOAD:                  5000,
>  
>  // TTL of the message sent to another device when sending a tab
>  NOTIFY_TAB_SENT_TTL_SECS:              1 * 3600, // 1 hour
>  
> +// Reasons behind sending collection_changed push notifications.

nit: Since it's used in only one file, we probably should move these to constants declared at the top of the file rather than injecting them into everybody who includes constants.js -- not to mention updating the modules.json.

::: services/sync/modules/engines/clients.js:467
(Diff revision 1)
>        }
>        const fxaDeviceId = this.getClientFxaDeviceId(id);
>        return fxaDeviceId ? acc.concat(fxaDeviceId) : acc;
>      }, []);
>      if (idsToNotify.length > 0) {
> -      this._notifyCollectionChanged(idsToNotify, NOTIFY_TAB_SENT_TTL_SECS);
> +      this._notifyOtherClientsModified(idsToNotify);

I guess it's intentional we don't await this promise? (I mean, we didn't before, but we probably at least should write a comment saying as much if it is intentional).

Another option would be to drop the promise in the functions themselves (instead of returning them) and comment there, since I do notice that we drop it above too.
Attachment #8917510 - Flags: review?(tchiovoloni) → review+
Thanks for the review Thom.
Landing this when the server changes are deployed in production.
Adding FxA train-97 deploy in Bug 1405505 as a blocker
Depends on: 1405505
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e4ce7eacfdfe
Add reason field to push messages sent with /notify. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/e4ce7eacfdfe
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.