[webkit-reviews] review denied: [Bug 185615] Storage Access API: Allow documents that have been granted storage access to also do a popup : [Attachment 340518] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Brent Fulgham <bfulgham at webkit.org>'s
request for review:
Bug 185615: Storage Access API: Allow documents that have been granted storage
access to also do a popup
https://bugs.webkit.org/show_bug.cgi?id=185615

Attachment 340518: Patch

https://bugs.webkit.org/attachment.cgi?id=340518&action=review




--- Comment #12 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 340518
  --> https://bugs.webkit.org/attachment.cgi?id=340518
Patch

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

> Source/WebCore/dom/Document.cpp:7619
> +	       document->enableOneTimeUserGesture();

This enables the user gesture way too early. A lot of things may get to use the
user gesture before the promise is actually resolved. This is because
promise::resolve() schedules a MicroTask. I think this should schedule a micro
task to enable the user gesture. See comment below for syntax.

> Source/WebCore/dom/Document.cpp:7622
> +	       document->postTask([documentReference =
WTFMove(documentReference)] (auto&) {

resolve() schedules a MicroTask, not a Task. Therefore, your order of execution
here is not necessarily guaranteed and this may well consume the user gesture
before we've actually run the JS as a result of resolving the promise.

I think this should be a
MicrotaskQueue::mainThreadQueue().append(VoidMicrotask([] {
  // do your thing.
});

> Source/WebCore/dom/Document.cpp:7624
> +		       document->consumeOneTimeUserGesture();

This is truly not a one time user gesture. I would suggest we either make it
truly one-time, or we rename it to "temporary" user gesture.

There are several ways we could fix this:
- Rename to "temporary", do not clear the user gesture on window.open(). Let
this micro task clear the user gesture.
- Keep calling it one time but then make it truly one time (every user gesture
check would clear the user gesture, not just the window.open API).
- Call it "OneTimeWindowOpen" and keep clearing it on window.open.

> Source/WebCore/page/DOMWindow.cpp:2385
> +	   document()->consumeOneTimeUserGesture();

Same issue as with the previous patch, this specializes window.open() for some
reason. Why would window.open() clear the one-time user gesture but not other
operations that require a user gesture?

Also note that the user gesture check is in DOMWindow::allowPopUp() I believe.
It is weird to do the check in one place but clear the user gesture in another
place.