Closed Bug 1205112 Opened 5 years ago Closed 5 years ago

Make `PushEvent.data` nullable

Categories

(Core :: DOM: Push Notifications, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

Details

Attachments

(1 file)

This lets us distinguish messages without an entity body from encrypted, blank messages.
Bug 1205112 - Make `PushEvent.data` nullable. r?mt
Attachment #8661523 - Flags: review?(martin.thomson)
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+
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)
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.
Flags: needinfo?(bugs)
Attachment #8661523 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9fafe1d5f32c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.