Make `pushsubscriptionchange` extendable

VERIFIED FIXED in Firefox 44

Status

()

defect
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 affected, firefox44 fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm
Attachment #8661518 - Flags: review?(nsm.nikhil)
Attachment #8661518 - Flags: review?(martin.thomson)
Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm
Assignee

Updated

4 years ago
Depends on: 1185544
Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

https://reviewboard.mozilla.org/r/19381/#review17349

This looks good if the spec simply makes the event an ExtendableEvent. Otherwise it may declare PushSubscriptionLostEvent inheriting ExtendableEvent (although I don't see the point since the subclass has no new fields/methods).

::: dom/push/test/test_subscription_change.html:4
(Diff revision 2)
> +Bug XXX: Make `pushsubscriptionchange` extendable.

Nit: Please fix the XXX here and below.

::: dom/push/test/test_subscription_change.html:38
(Diff revision 2)
> +      iframe.src = "http://mochi.test:8888/tests/dom/push/test/frame.html";

Please use a relative URL.

::: dom/push/test/worker.js:70
(Diff revision 2)
> +  ).then(([client]) => {
> +    client.postMessage({type: "changed", okay: "yes"});
> +  }));

This will fail rather silently if client is undefined for whatever reason, can you add a catch that dumps pretty loudly? Thanks

::: dom/workers/ServiceWorkerManager.cpp:2373
(Diff revision 2)
> -    WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope();
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());

ExtendableEvent::Constructor() has a second form that takes an event target, so you don't have to do this hack. See DispatchLifecycleEvent.

::: dom/workers/ServiceWorkerManager.cpp:2381
(Diff revision 2)
> +      ExtendableEvent::Constructor(globalObj,
> +                                   NS_LITERAL_STRING("pushsubscriptionchange"),
> +                                   init, result);

The second form also skips having to deal with `result`
Attachment #8661518 - Flags: review?(nsm.nikhil) → review+
Status: NEW → ASSIGNED
Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

https://reviewboard.mozilla.org/r/19381/#review17421

::: dom/push/PushService.jsm:1025
(Diff revision 2)
> -    this[aMessage.name.slice("Push:".length).toLowerCase()](pageRecord, mm);
> +    this[name.toLowerCase()](pageRecord, mm);

For later...

```
this[name.toLowerCase()](pageRecord)
  .then(message => {
    message.requestID = pageRecord.requestID;
    mm.sendAsyncMessage("PushService:" + name + ":OK", message);
  }, error => {
    let message = {
      requestID: pageRecord.requestID,
      error: error
    };
    mm.sendAsyncMessage("PushService:" + name + ":KO", message);
  });
```

Then reset() can just route to dropRegistrations()

::: dom/push/test/frame.html:8
(Diff revision 2)
> +    function waitOnSubscriptionChange()
> +    {
> +      var p = new Promise(function(res, rej) {
> +        function onMessage(e) {
> +          if (e.data.type == "changed") {
> +            navigator.serviceWorker.removeEventListener("message", onMessage);
> +            (e.data.okay == "yes" ? res : rej)(e.data);
> +          }
> +        }
> +        navigator.serviceWorker.addEventListener("message", onMessage);
> +      });
> +      return p;
> +    }

This just duplicates waitOnPushmessage.  Refactor please.

::: dom/push/test/test_subscription_change.html:4
(Diff revision 2)
> +Bug XXX: Make `pushsubscriptionchange` extendable.

Bug number

::: dom/push/test/test_subscription_change.html:11
(Diff revision 2)
> +  <title>Test for Bug XXX</title>

XXX

::: dom/push/test/test_subscription_change.html:16
(Diff revision 2)
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=XXX">Mozilla Bug XXX</a>

XXX

::: dom/push/test/test_subscription_change.html:41
(Diff revision 2)
> +        res(swr)

arrow function or semi-colon.

Why do you resolve with the service worker registration?  You saved that to a global already...

::: dom/push/test/test_subscription_change.html:42
(Diff revision 2)
> +      }

Missing semi-colon.

::: dom/workers/ServiceWorkerManager.cpp:2373
(Diff revision 2)
> -    WorkerGlobalScope* globalScope = aWorkerPrivate->GlobalScope();
> +    GlobalObject globalObj(aCx, aWorkerPrivate->GlobalScope()->GetWrapper());

Two lines and assert for null on the return from aWorkPrivate->GlobalScope().
Attachment #8661518 - Flags: review?(martin.thomson) → review+
https://reviewboard.mozilla.org/r/19381/#review17349

> This will fail rather silently if client is undefined for whatever reason, can you add a catch that dumps pretty loudly? Thanks

Do workers have access to `dump`? The test will time out if it doesn't receive a response, so we'll still know something is wrong...
Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm
(In reply to Kit Cambridge [:kitcambridge] from comment #5)
> Do workers have access to `dump`? The test will time out if it doesn't
> receive a response, so we'll still know something is wrong...

Yes, dump() works in workers.
Assignee

Updated

4 years ago
Blocks: 1219064
Comment on attachment 8661518 [details]
MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt

Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt
Attachment #8661518 - Attachment description: MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt,nsm → MozReview Request: Bug 1205109 - Make `pushsubscriptionchange` extendable. r?mt
Kit, is this review request right?  It looks like you are rolling back all the changes.
(In reply to Martin Thomson [:mt:] from comment #14)
> Kit, is this review request right?  It looks like you are rolling back all
> the changes.

It is. `DispatchExtendableEventOnWorkerScope` really cleans up the logic. I did roll back the test and `Push:Reset` addition. Now that we have the permission observer, I think the test in bug 1219064 is cleaner.

Comment 17

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cf81a62b5a37
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Posted patch 1.patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: Bug 1233387.
[User impact if declined]: The `pushsubscriptionchange` event may not keep the worker alive long enough to request a new subscription. Consequently, sites with expired push subscriptions may not be able to request new ones, and the user won't receive push messages from that site.
[Describe test coverage new/current, TreeHerder]: Covered by existing mochitests.
[Risks and why]: Low risk. On Nightly for more than six weeks; straightforward change to the worker code.
[String/UUID change made/needed]: None.
Attachment #8661518 - Attachment is obsolete: true
Attachment #8700917 - Flags: review+
Attachment #8700917 - Flags: approval-mozilla-beta?
Comment on attachment 8700917 [details] [diff] [review]
1.patch

This has been on Central for 6 weeks, seems safe to uplift to Beta44.
Attachment #8700917 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified Beta 44.0b4
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.