When calling query() from a a non-fully active document, it should rejects with a InvalidStateError. Similarly, non-fully active documents shouldn't receive events.
Created attachment 452717 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment on attachment 452717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452717&action=review > Source/WebCore/Modules/permissions/Permissions.cpp:88 > + if (is<Document>(context) && !downcast<Document>(*context).isFullyActive()) { https://w3c.github.io/permissions/#query-method is not clear about this check, is there a corresponding PR that should be merged? Are other browsers already implementing this check? > Source/WebCore/Modules/permissions/Permissions.cpp:94 > promise.reject(Exception { InvalidStateError, "The context is invalid"_s }); Maybe we should do this check before the fully active one. > LayoutTests/imported/w3c/ChangeLog:10 > + * web-platform-tests/permissions/resources/empty.html: Added. Are these new WPT tests that need to be upstreamed or are they already merged? > LayoutTests/imported/w3c/web-platform-tests/permissions/non-fully-active.https-expected.txt:6 > +TIMEOUT Permission change events shouldn't fire on non-fully active document Test timed out We usually do not like tests to timeout. Can the test be updated to check for onchange and fail early if not present?
(In reply to youenn fablet from comment #3) > Comment on attachment 452717 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452717&action=review > > > Source/WebCore/Modules/permissions/Permissions.cpp:88 > > + if (is<Document>(context) && !downcast<Document>(*context).isFullyActive()) { > > https://w3c.github.io/permissions/#query-method is not clear about this > check, is there a corresponding PR that should be merged? Sorry I should have pasted the link: https://github.com/w3c/permissions/pull/365 > Are other browsers already implementing this check? See links to other implementations. > > Source/WebCore/Modules/permissions/Permissions.cpp:94 > > promise.reject(Exception { InvalidStateError, "The context is invalid"_s }); > > Maybe we should do this check before the fully active one. > > > LayoutTests/imported/w3c/ChangeLog:10 > > + * web-platform-tests/permissions/resources/empty.html: Added. > > Are these new WPT tests that need to be upstreamed or are they already > merged? PR at https://github.com/web-platform-tests/wpt/pull/32921 > > LayoutTests/imported/w3c/web-platform-tests/permissions/non-fully-active.https-expected.txt:6 > > +TIMEOUT Permission change events shouldn't fire on non-fully active document Test timed out > > We usually do not like tests to timeout. Can the test be updated to check > for onchange and fail early if not present?
OK, this sounds good to me, a few nits then: - Can we mention the PRS in the change log? - Can we update the test to fail early instead of timing out? - Can we check that there is a context before checking whether active?
Created attachment 452777 [details] Patch
(In reply to youenn fablet from comment #5) > OK, this sounds good to me, a few nits then: > - Can we mention the PRS in the change log? Done. > - Can we update the test to fail early instead of timing out? Done. > - Can we check that there is a context before checking whether active? Done. And thanks! I was wondering about this also.
Comment on attachment 452777 [details] 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
Created attachment 452832 [details] Patch
Comment on attachment 452832 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452832&action=review > Source/WebCore/Modules/permissions/PermissionStatus.cpp:72 > + auto* context = scriptExecutionContext(); Not really needed > Source/WebCore/Modules/permissions/PermissionStatus.cpp:73 > + if (auto document = dynamicDowncast<Document>(context); !document->isFullyActive()) I would use auto* to match Permissions::query.
Created attachment 452834 [details] Patch
Committed r290301 (247624@main): <https://commits.webkit.org/247624@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 452834 [details].
<rdar://problem/89288354>