Bug 238188

Created attachment 455337 [details]
Patch

<rdar://90590937>

Comment on attachment 455337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=455337&action=review

> Source/WebCore/Modules/push-api/PushManager.cpp:140
> +            auto* window = document.frame()->window();

check frame is null?

> Source/WebCore/Modules/push-api/PushManager.cpp:141
> +            if (!window || !window->consumeTransientActivation()) {

I would be tempted to do that check before enqueuing a task, but I guess this is more consistent the way you did it with the way the algorithm is done.
The spec is a bit loose there, I filed https://github.com/w3c/push-api/issues/346 to cover this.
I guess the transient activation is implicitly done as part of step 8.
This is potentially racy, for instance:
- script checks subscribe permission (it is allowed).
- before this event loop task is run, permission is changed to prompt
- subscribe fails

Is it fully consistent with Chrome and Firefox, aka is transient activation only checked when permission is neither granted nor denied?


> LayoutTests/ChangeLog:8
> +        Add test cases to make sure that showing a permission prompt consumes a user gesture.

Can you add a test validating that calling subscribe out of user gesture 'works' if permission is denied or permission is granted?

Created attachment 455461 [details]
Patch

(In reply to youenn fablet from comment #3)
> Comment on attachment 455337 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455337&action=review
> 
> > Source/WebCore/Modules/push-api/PushManager.cpp:140
> > +            auto* window = document.frame()->window();
> 
> check frame is null?

Fixed in the patch to land.

> > Source/WebCore/Modules/push-api/PushManager.cpp:141
> > +            if (!window || !window->consumeTransientActivation()) {
> 
> I would be tempted to do that check before enqueuing a task, but I guess
> this is more consistent the way you did it with the way the algorithm is
> done.
> The spec is a bit loose there, I filed
> https://github.com/w3c/push-api/issues/346 to cover this.
> I guess the transient activation is implicitly done as part of step 8.
> This is potentially racy, for instance:
> - script checks subscribe permission (it is allowed).
> - before this event loop task is run, permission is changed to prompt
> - subscribe fails

Yeah, it's not clear which way is better, I just went with this way since it seemed clearer and closer to the algorithm the spec wants us to use.

> Is it fully consistent with Chrome and Firefox, aka is transient activation
> only checked when permission is neither granted nor denied?

I checked Chromium, and as far as I can tell from inspection, they only check the transient activation flag in the case that they show a prompt.

> > LayoutTests/ChangeLog:8
> > +        Add test cases to make sure that showing a permission prompt consumes a user gesture.
> 
> Can you add a test validating that calling subscribe out of user gesture
> 'works' if permission is denied or permission is granted?

I think these scenarios are already tested in subscribe-grant-permissions.html and subscribe-deny-permissions.html in the calls to testDocumentSubscribeWithoutUserGesture. I did update the logic in the test a bit to make sure that we are hitting the exact NotAllowedError we expect rather than any NotAllowedError.

Created attachment 455463 [details]
Patch for landing

Committed r291737 (248769@main): <https://commits.webkit.org/248769@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455463 [details].