Make `PushEvent.data` nullable

RESOLVED FIXED in Firefox 43

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kitcambridge, Assigned: kitcambridge)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

This lets us distinguish messages without an entity body from encrypted, blank messages.
Created attachment 8661523 [details]
MozReview Request: Bug 1205112 - Make `PushEvent.data` nullable. r?mt

Bug 1205112 - Make `PushEvent.data` nullable. r?mt
Attachment #8661523 - Flags: review?(martin.thomson)
Depends on: 1185544

Updated

3 years ago
Status: NEW → ASSIGNED
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.
Attachment #8661523 - Flags: review?(martin.thomson) → review+
Comment on attachment 8661523 [details]
MozReview Request: Bug 1205112 - Make `PushEvent.data` nullable. r?mt

Bug 1205112 - Make `PushEvent.data` nullable. r?mt
...And this WebIDL change, too. :-) Thanks!
Flags: needinfo?(bugs)

Comment 6

3 years ago
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?
(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.
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.

Updated

3 years ago
Flags: needinfo?(bugs)
Attachment #8661523 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9fafe1d5f32c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.