Summary: | Add stubs for Permissions API | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | annulen, beidson, benjamin, calvaris, cdumez, dvpdiner2, esprehn+autocc, ews-watchlist, gyuyoung.kim, japhet, kangil.han, kondapallykalyan, marcos, rniwa, ryuan.choi, sam, sergio, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 229504, 229590 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Sihui Liu
2021-08-20 09:31:01 PDT
Created attachment 435998 [details]
Patch
Created attachment 436000 [details]
Patch
Created attachment 436001 [details]
Patch
Created attachment 436021 [details]
Patch
If at all possible, it would be great if there was a bugzilla component for the Permissions API. Alternatively, a meta bug would be great. (I edit the spec, so this would really help with issue tracking, etc.) If at all possible, it would be great if there was a bugzilla component for the Permissions API. Alternatively, a meta bug would be great. (I edit the spec, so this would really help with issue tracking, etc.) Created attachment 436534 [details]
Patch
Created attachment 436547 [details]
Patch
Created attachment 436550 [details]
Patch
Created attachment 436554 [details]
Patch
Comment on attachment 436554 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436554&action=review > Source/WebCore/Modules/permissions/PermissionController.h:47 > +class DummyPermissionController : public PermissionController { class could be marked as final. > Source/WebCore/Modules/permissions/PermissionStatus.h:60 > + const ClientOrigin& origin() final { return m_origin; } should this be marked as const? > Source/WebCore/Modules/permissions/PermissionStatus.h:61 > + const PermissionDescriptor& descriptor() final { return m_descriptor; } ditto. Created attachment 436558 [details]
Patch
Created attachment 436566 [details]
Patch
Created attachment 436575 [details]
Patch
Created attachment 436578 [details]
Patch
Created attachment 436583 [details]
Patch
Created attachment 436586 [details]
Patch
Created attachment 436598 [details]
Patch
Comment on attachment 436598 [details] Patch Did not have time for a full review, some comments below. My main comment is to make sure that we structure window/worker specific code appropriately. View in context: https://bugs.webkit.org/attachment.cgi?id=436598&action=review > Source/WebCore/Modules/permissions/PermissionController.h:28 > +#include "PermissionObserver.h" Can be removed, since class PermissionObserver is added > Source/WebCore/Modules/permissions/PermissionName.idl:34 > + "background-sync", Should we restrict to the values we actually support? > Source/WebCore/Modules/permissions/PermissionObserver.h:28 > +#include "ClientOrigin.h" We could forward declare it > Source/WebCore/Modules/permissions/PermissionStatus.cpp:46 > +PermissionStatus::PermissionStatus(ScriptExecutionContext& context, PermissionState state, PermissionDescriptor descriptor) Is it worker and document? Should we restrict to Document&? Or should we add a fixme for worker support? > Source/WebCore/Modules/permissions/PermissionStatus.idl:35 > + attribute EventHandler onchange; What about GC in case onchange event handler is set? > Source/WebCore/Modules/permissions/Permissions.cpp:74 > + auto parameterDescriptor = convert<IDLDictionary<PermissionDescriptor>>(*context->globalObject(), permissionDescriptorValue.get()); Why do we need to do the conversion here instead of letting the binding generator handle it? > Source/WebCore/Modules/permissions/Permissions.cpp:81 > + auto* document = frame? frame->document() : nullptr; This is probably specific to window and not workers. It seems we should either abstract the code, for instance by using ScriptExecutionContext or restrict permission API to window only for now? (In reply to youenn fablet from comment #19) > Comment on attachment 436598 [details] > Patch > > Did not have time for a full review, some comments below. > My main comment is to make sure that we structure window/worker specific > code appropriately. Will try structuring it for worker code. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=436598&action=review > > > Source/WebCore/Modules/permissions/PermissionController.h:28 > > +#include "PermissionObserver.h" > > Can be removed, since class PermissionObserver is added Sure. > > > Source/WebCore/Modules/permissions/PermissionName.idl:34 > > + "background-sync", > > Should we restrict to the values we actually support? Sure, I will remove some values. > > > Source/WebCore/Modules/permissions/PermissionObserver.h:28 > > +#include "ClientOrigin.h" > > We could forward declare it Okay. > > > Source/WebCore/Modules/permissions/PermissionStatus.cpp:46 > > +PermissionStatus::PermissionStatus(ScriptExecutionContext& context, PermissionState state, PermissionDescriptor descriptor) > > Is it worker and document? > Should we restrict to Document&? > Or should we add a fixme for worker support? The spec says worker and document but we can do document only first. I will restrict to document in this patch and addd a comment for worker support. > > > Source/WebCore/Modules/permissions/PermissionStatus.idl:35 > > + attribute EventHandler onchange; > > What about GC in case onchange event handler is set? What do you mean? Do you mean to remove the observer and stop dispatching change event in stop()? > > > Source/WebCore/Modules/permissions/Permissions.cpp:74 > > + auto parameterDescriptor = convert<IDLDictionary<PermissionDescriptor>>(*context->globalObject(), permissionDescriptorValue.get()); > > Why do we need to do the conversion here instead of letting the binding > generator handle it? PermissionDescriptor can be extended, so the parameter is JSObject. The next step here should be converting it to a TypedPermissionDescriptor based on name. But we don't add any TypedPermissionDescriptor now. > > > Source/WebCore/Modules/permissions/Permissions.cpp:81 > > + auto* document = frame? frame->document() : nullptr; > > This is probably specific to window and not workers. > It seems we should either abstract the code, for instance by using > ScriptExecutionContext or restrict permission API to window only for now? Will try abstracting it and limit to window access in this patch. Created attachment 436664 [details]
Patch
Comment on attachment 436664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436664&action=review > Source/WebCore/Modules/permissions/PermissionStatus.cpp:67 > +void PermissionStatus::stateChanged(PermissionState newState) > +{ > + if (m_state != newState) > + queueTaskToDispatchEvent(*this, TaskSource::Permission, Event::create(eventNames().changeEvent, Event::CanBubble::No, Event::IsCancelable::No)); > +} This event dispatching mechanism isn't sound from GC perspective. We don't have anything in place to keep the JS wrapper of PermissionStatus alive until this function is called. Comment on attachment 436664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436664&action=review >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:67 >> +} > > This event dispatching mechanism isn't sound from GC perspective. > We don't have anything in place to keep the JS wrapper of PermissionStatus alive until this function is called. To try and clarify Ryosuke's comment. This is the right way to queue a task to dispatch and event. However, you'll also need to override ActiveDOMObject::virtualHasPendingActivity() to return true as long as the PermissionStatus object may dispatch events in the future, to keep the JS wrapper alive until stateChanged() gets called. Comment on attachment 436664 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436664&action=review >>> Source/WebCore/Modules/permissions/PermissionStatus.cpp:67 >>> +} >> >> This event dispatching mechanism isn't sound from GC perspective. >> We don't have anything in place to keep the JS wrapper of PermissionStatus alive until this function is called. > > To try and clarify Ryosuke's comment. This is the right way to queue a task to dispatch and event. > > However, you'll also need to override ActiveDOMObject::virtualHasPendingActivity() to return true as long as the PermissionStatus object may dispatch events in the future, to keep the JS wrapper alive until stateChanged() gets called. You basically need to implement the language at https://w3c.github.io/permissions/#permissionstatus-gc since a PermissionStatus::virtualHasPendingActivity() override. (In reply to Chris Dumez from comment #25) > Comment on attachment 436664 [details] > Patch > > You basically need to implement the language at > https://w3c.github.io/permissions/#permissionstatus-gc since a > PermissionStatus::virtualHasPendingActivity() override. An alternative is to use makePendingActivity and hold onto the activity object while we're waiting for something. That might work better for permission API since we'd be typically asking for UI process to get a permission? Created attachment 436679 [details]
Patch
Created attachment 436681 [details]
Patch
Comment on attachment 436679 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436679&action=review > Source/WebCore/Modules/permissions/PermissionStatus.cpp:79 > + return hasEventListeners(eventNames().changeEvent) || m_hasPendingActivity; This isn't gonna work. This function will return true forever as soon as someone added an event listener for "change". Created attachment 436695 [details]
Patch
Created attachment 436699 [details]
Patch
Comment on attachment 436699 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436699&action=review > Source/WebCore/ChangeLog:3 > + Add stubs for Permissions API This seems like it is doing way more than adding what we normally call stubs. > Source/WebCore/Modules/permissions/PermissionName.h:50 > +template<> struct EnumTraits<WebCore::PermissionName> { This file needs to include wtf/EnumTraits.h. > Source/WebCore/Modules/permissions/PermissionName.idl:29 > + EnabledBySetting=PermissionsAPI, I am not sure if there is any benefit to adding a EnabledBySetting to an enum. I don't believe it does anything. > Source/WebCore/Modules/permissions/PermissionState.idl:29 > + EnabledBySetting=PermissionsAPI, Same comment about unnecessary EnabledBySetting. Created attachment 436760 [details]
Patch
Comment on attachment 436760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436760&action=review > Source/WebKit/ChangeLog:9 > + Set up basic infrastrcture of permission getting and setting. Typo: infrastrcture > Source/WebCore/Modules/permissions/PermissionController.h:38 > +class PermissionController : public ThreadSafeRefCounted<PermissionController> { Why ThreadSafeRefCounted? Do we really expect such object to be passed to different threads? > Source/WebCore/Modules/permissions/PermissionStatus.cpp:54 > + m_origin = ClientOrigin { context.topOrigin().data(), originData }; WTFMove(originData) > Source/WebCore/Modules/permissions/PermissionStatus.cpp:69 > + m_hasPendingActivity = true; What is this flag? queueTaskToDispatchEvent() takes care of keeping your wrapper alive. This flag seems pointless. > Source/WebCore/Modules/permissions/PermissionStatus.cpp:80 > + if (m_hasPendingActivity) This flag seems pointless > Source/WebCore/Modules/permissions/PermissionStatus.cpp:85 > + if (context && context->isDocument()) return is<Document>(context) && downcast<Document>(*context).hasBrowsingContext(); > Source/WebCore/Modules/permissions/PermissionStatus.cpp:93 > +void PermissionStatus::dispatchEvent(Event& event) This override doesn't seem necessary. > Source/WebCore/Modules/permissions/PermissionStatus.cpp:103 > + m_hasRelevantEventListener = hasEventListeners(eventNames().changeEvent); Can we call this flag hasChangeEventListener for clarity since there is only one relevant event listener at the moment? Comment on attachment 436760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436760&action=review >> Source/WebKit/ChangeLog:9 >> + Set up basic infrastrcture of permission getting and setting. > > Typo: infrastrcture Will change. >> Source/WebCore/Modules/permissions/PermissionController.h:38 >> +class PermissionController : public ThreadSafeRefCounted<PermissionController> { > > Why ThreadSafeRefCounted? Do we really expect such object to be passed to different threads? We will support this in WorkerNavigator. A way is to subclass this and introduce WorkerPermissionController and MainThreadPermissionController (similar to CacheStorageConnection), so I make it ThreadSafeRefCounted. >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:54 >> + m_origin = ClientOrigin { context.topOrigin().data(), originData }; > > WTFMove(originData) Will change. >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:69 >> + m_hasPendingActivity = true; > > What is this flag? queueTaskToDispatchEvent() takes care of keeping your wrapper alive. This flag seems pointless. I see; will remove. >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:80 >> + if (m_hasPendingActivity) > > This flag seems pointless Will remove. >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:85 >> + if (context && context->isDocument()) > > return is<Document>(context) && downcast<Document>(*context).hasBrowsingContext(); I thought we want to always return true for workers if m_hasRelevantEventListener is true? >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:93 >> +void PermissionStatus::dispatchEvent(Event& event) > > This override doesn't seem necessary. Will remove. >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:103 >> + m_hasRelevantEventListener = hasEventListeners(eventNames().changeEvent); > > Can we call this flag hasChangeEventListener for clarity since there is only one relevant event listener at the moment? Will update. Comment on attachment 436760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436760&action=review > Source/WebCore/Modules/permissions/PermissionStatus.cpp:83 > + if (m_hasRelevantEventListener) { I would do an early return: if (!m_hasChangeEventListener) return false; > Source/WebCore/Modules/permissions/Permissions.cpp:71 > + promise.reject(Exception { InvalidStateError, "The context is invalid" }); "The context is invalid"_s > Source/WebCore/Modules/permissions/Permissions.h:45 > +class Permissions final : public RefCounted<Permissions> { Why final? This is not a virtual class. > Source/WebKit/UIProcess/WebPageProxy.cpp:8497 > +void WebPageProxy::requestPermission(const WebCore::ClientOrigin&, const WebCore::PermissionDescriptor&, CompletionHandler<void(WebCore::PermissionState)>&& completionHandler) WebCore:: is unnecessary in this file. > Source/WebKit/UIProcess/WebPageProxy.h:2121 > + void requestPermission(const WebCore::ClientOrigin&, const WebCore::PermissionDescriptor&, CompletionHandler<void(WebCore::PermissionState)>&&); In WebCore, you pass PermissionDescriptor by value (which seems OK since this is basically just an enum at the moment). However, you're passing it by reference here. I think we should be consistent. > Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:97 > + while (!m_requests.isEmpty()) { Should this function be named tryProcessingRequests() then? Doesn't look like it only processes one request. > Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:105 > + auto takenRequest = m_requests.takeFirst(); We don't need this local variable. Comment on attachment 436760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436760&action=review >>> Source/WebCore/Modules/permissions/PermissionStatus.cpp:85 >>> + if (context && context->isDocument()) >> >> return is<Document>(context) && downcast<Document>(*context).hasBrowsingContext(); > > I thought we want to always return true for workers if m_hasRelevantEventListener is true? Oh, I misread. My point was really that you could omit the null check for context: if (is<Document>(context)) return downcast<Document>(*context).hasBrowsingContext(); return true; Comment on attachment 436760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436760&action=review >> Source/WebCore/Modules/permissions/PermissionStatus.cpp:83 >> + if (m_hasRelevantEventListener) { > > I would do an early return: > if (!m_hasChangeEventListener) > return false; Sounds good. >> Source/WebCore/Modules/permissions/Permissions.cpp:71 >> + promise.reject(Exception { InvalidStateError, "The context is invalid" }); > > "The context is invalid"_s Will add. >> Source/WebCore/Modules/permissions/Permissions.h:45 >> +class Permissions final : public RefCounted<Permissions> { > > Why final? This is not a virtual class. Oops. >> Source/WebKit/UIProcess/WebPageProxy.cpp:8497 >> +void WebPageProxy::requestPermission(const WebCore::ClientOrigin&, const WebCore::PermissionDescriptor&, CompletionHandler<void(WebCore::PermissionState)>&& completionHandler) > > WebCore:: is unnecessary in this file. Will remove. >> Source/WebKit/UIProcess/WebPageProxy.h:2121 >> + void requestPermission(const WebCore::ClientOrigin&, const WebCore::PermissionDescriptor&, CompletionHandler<void(WebCore::PermissionState)>&&); > > In WebCore, you pass PermissionDescriptor by value (which seems OK since this is basically just an enum at the moment). However, you're passing it by reference here. I think we should be consistent. Will change the value to reference. >> Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:97 >> + while (!m_requests.isEmpty()) { > > Should this function be named tryProcessingRequests() then? Doesn't look like it only processes one request. Okay. >> Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:105 >> + auto takenRequest = m_requests.takeFirst(); > > We don't need this local variable. Okay. Created attachment 436784 [details]
Patch
Comment on attachment 436784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436784&action=review r=me > Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.cpp:65 > + auto permissionEntries = m_cachedPermissionEntries.get(origin); Please avoid double-hash table look-up with contains and then get(). (you can use find()). Created attachment 436789 [details]
Patch
Committed r281771 (241108@main): <https://commits.webkit.org/241108@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436789 [details]. Comment on attachment 436789 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436789&action=review > Source/WebCore/Modules/permissions/PermissionController.h:39 > + WTF_MAKE_FAST_ALLOCATED; Not needed since this subclasses ThreadSafeRefCounted. > Source/WebCore/Modules/permissions/PermissionController.h:45 > + virtual void removeObserver(PermissionObserver&) = 0; I recommend making the constructor protected: ``` protected: PermissionController() = default; ``` > Source/WebCore/Modules/permissions/PermissionController.h:49 > + PermissionState query(ClientOrigin&&, PermissionDescriptor&&) final { return PermissionState::Denied; } The constructor should be private and this needs a factory create() function since this subclasses ThreadSafeRefCounted. > Source/WebKit/WebProcess/WebCoreSupport/WebPermissionController.h:40 > + explicit WebPermissionController(WebPage&); This constructor should be private and there should be a public create() factory function since this subclasses ThreadSafeRefCounted. |