Re: [w3c/push-api] Rewrite .subscribe() to fix various bugs (PR #350)

@martinthomson commented on this pull request.



> +        <li>If the user agent requires |options|'s {{PushSubscriptionOptionsInit/userVisibleOnly}}
+        member to be `true`, return [=a promise rejected with=] an {{"NotAllowedError"}}
+        {{DOMException}}.

This would always reject if userVisibleOnly had to be true, rather than just when it is false.

```suggestion
        <li>If the |options| argument has a {{PushSubscriptionOptionsInit/userVisibleOnly}} value set to `false` and 
        the user agent requires it to be `true`, return [=a promise rejected with=] an {{"NotAllowedError"}}
        {{DOMException}}.
```

I'm not 100% on the bit where we this leans on defaults, your next step seems to do that though.

> +            <li>Ensure that |options|'s {{PushSubscriptionOptionsInit/applicationServerKey}}
+            describes a valid point on the P-256 curve. If its value is invalid, return [=a promise
+            rejected with=] an {{"InvalidAccessError"}} {{DOMException}}.

In terms of synchronous steps, this is not one that can be done synchronously in all implementations as it is non-trivial in the sense that it invokes cryptography.  It is possible that this difference would be observable.

(I checked the gecko implementation and I can't see where we implement this check, so season accordingly.  We probably implement this check on the push service instead, which we should probably fix, but there you go...)



> +            <li>If |subscription| is an error, [=queue a global task=] on the [=networking task
+            source=] using |global| to [=reject=] |promise| with an {{"AbortError"}}
+            {{DOMException}} and terminate these steps.

How can *subscription* be an error here?  The outcome of an attempt to retrieve it might return an error, but the *subscription* will be a subscription, right?

Maybe the answer is that you need to invoke a process for obtaining *subscription* from *sw*, which could then return an error.  In that case, you won't learn that *sw* is already subscribed, so you should rearrange these steps along the lines of:

1. Let *subscription* be the result of obtaining the current push subscription from *sw*.
2. If *subscription* is an error, (...this step...reject the promise)
3. If *subscription* exists, run the following substeps:
    a. (...the other checks here...)
    b. (...resolve the promise.)
4. (Otherwise) (...make a subscription)

-- 
Reply to this email directly or view it on GitHub:
https://github.com/w3c/push-api/pull/350#pullrequestreview-996115570
You are receiving this because you are subscribed to this thread.

Message ID: <w3c/push-api/pull/350/review/996115570@github.com>