Make `PushEvent.data` nullable

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

Trunk
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

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

Comment 1

4 years ago
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)
(Assignee)

Updated

4 years ago
Depends on: 1185544
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+
(Assignee)

Comment 3

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

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

Comment 5

4 years ago
...And this WebIDL change, too. :-) Thanks!
Flags: needinfo?(bugs)
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?
(Assignee)

Comment 7

4 years ago
(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.
Flags: needinfo?(bugs)
Attachment #8661523 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9fafe1d5f32c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.