[webkit-reviews] review granted: [Bug 236952] Permission API: handle non-fully active documents : [Attachment 452777] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Marcos Caceres
<marcos at marcosc.com>'s request for review:
Bug 236952: Permission API: handle non-fully active documents
https://bugs.webkit.org/show_bug.cgi?id=236952

Attachment 452777: Patch

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




--- Comment #8 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 452777
  --> https://bugs.webkit.org/attachment.cgi?id=452777
Patch

LGTM, a few comments below.

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

> Source/WebCore/Modules/permissions/PermissionStatus.cpp:75
> +	   if (!document.isFullyActive())

We have the new dynamicDowncast we could use:
if (auto* document = dynamicDowncast<Document>(context);
!document->isFullyActive())
    return;

Also, this makes the case of iframes detached-then-reattached potentially
complex to handle for developers.
Say we query permission before detaching (and get granted), we detach, we
change permission to prompt, we reattach the frame and we query permission
again.
The two permission objects would have different state. This seems somewhat
inconvenient for web developers (although simpler for implementors).
There could be a case where web page hangs on the first permission object
state.

> Source/WebCore/Modules/permissions/Permissions.cpp:92
> +    if (is<Document>(context) &&
!downcast<Document>(*context).isFullyActive()) {

Ditto for dynamicDowncast