1205112 - Make `PushEvent.data` nullable

Status: RESOLVED FIXED

(Core :: DOM: Push Subscriptions, defect)

Description

[Lina Butler [:lina]]

This lets us distinguish messages without an entity body from encrypted, blank messages.

Comment 1

[Lina Butler [:lina]]

Attachment: MozReview Request: Bug 1205112 - Make `PushEvent.data` nullable. r?mt

Bug 1205112 - Make `PushEvent.data` nullable. r?mt

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8661523 [details]
MozReview Request: Bug 1205112 - Make `PushEvent.data` nullable. r?mt

https://reviewboard.mozilla.org/r/19385/#review17423

::: dom/webidl/PushEvent.webidl:14
(Diff revision 1)
> -  readonly attribute PushMessageData data;
> +  readonly attribute PushMessageData? data;

Find yourself a DOM peer for this.

::: dom/workers/ServiceWorkerManager.cpp:2305
(Diff revision 1)
>    SendPushEventRunnable(
>      WorkerPrivate* aWorkerPrivate,
> +    nsMainThreadPtrHandle<ServiceWorker>& aServiceWorker)
> +      : WorkerRunnable(aWorkerPrivate, WorkerThreadModifyBusyCount)
> +      , mHasData(false)
> +      , mServiceWorker(aServiceWorker)

Use a delegated constructor here.  (Yes, I know that it means having yet another constructor, make it private.)

::: dom/workers/ServiceWorkerManager.cpp:2443
(Diff revision 1)
> -    new SendPushEventRunnable(serviceWorker->GetWorkerPrivate(), data,
> +  } else {
> +    r = new SendPushEventRunnable(serviceWorker->GetWorkerPrivate(),
> -                              serviceWorkerHandle);
> +                                  serviceWorkerHandle);
> +  }

Please add an assert on optional_argc == 0 here.

Comment 3

[Lina Butler [:lina]]

Comment on attachment 8661523 [details]
MozReview Request: Bug 1205112 - Make `PushEvent.data` nullable. r?mt

Bug 1205112 - Make `PushEvent.data` nullable. r?mt

Comment 4

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3304b18b01c6

Comment 5

[Lina Butler [:lina]]

...And this WebIDL change, too. :-) Thanks!

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

The spec has non-nullable .data, and the spec seems to even define that if no data was passed, treat it as empty sequence of data.
So, why we want this change?

Comment 7

[Lina Butler [:lina]]

(In reply to Olli Pettay [:smaug] from comment #6)
> The spec has non-nullable .data, and the spec seems to even define that if
> no data was passed, treat it as empty sequence of data.
> So, why we want this change?

Currently, a service worker can't distinguish between an authenticated blank message and a message without a payload. The latter could indicate a Simple Push-style "ping," or a signal that the data was stripped by the Push server. We're considering this for our server to cut down on storage for stale subscriptions.

I opened https://github.com/w3c/push-api/issues/161 to track this; Martin put it on his list to write up.

Comment 8

[Martin Thomson [:mt:]]

This was agreed with Google a long while ago.  But I'm slow.

https://github.com/w3c/push-api/pull/166 awaits review from my co-editor.

Comment 9

[Lina Butler [:lina]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bf147cea14c

Comment 10

[Lina Butler [:lina]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fafe1d5f32c636106e3adf3d9dd17e41779f7b0
Bug 1205112 - Make `PushEvent.data` nullable. r=mt,smaug

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9fafe1d5f32c