Bug 229339 – Add stubs for Permissions API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 435998 [details]
Patch

[Sihui Liu]

Created attachment 436000 [details]
Patch

[Sihui Liu]

Created attachment 436001 [details]
Patch

[Sihui Liu]

Created attachment 436021 [details]
Patch

[Marcos Caceres]

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.)

[Marcos Caceres]

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.)

[Sihui Liu]

Created attachment 436534 [details]
Patch

[Sihui Liu]

Created attachment 436547 [details]
Patch

[Sihui Liu]

Created attachment 436550 [details]
Patch

[Sihui Liu]

Created attachment 436554 [details]
Patch

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 436558 [details]
Patch

[Sihui Liu]

Created attachment 436566 [details]
Patch

[Sihui Liu]

Created attachment 436575 [details]
Patch

[Sihui Liu]

Created attachment 436578 [details]
Patch

[Sihui Liu]

Created attachment 436583 [details]
Patch

[Sihui Liu]

Created attachment 436586 [details]
Patch

[Sihui Liu]

Created attachment 436598 [details]
Patch

[youenn fablet]

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?

[Radar WebKit Bug Importer]

<rdar://problem/82442205>

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 436664 [details]
Patch

[Ryosuke Niwa]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Ryosuke Niwa]

(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?

[Sihui Liu]

Created attachment 436679 [details]
Patch

[Sihui Liu]

Created attachment 436681 [details]
Patch

[Ryosuke Niwa]

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".

[Sihui Liu]

Created attachment 436695 [details]
Patch

[Sihui Liu]

Created attachment 436699 [details]
Patch

[Sam Weinig]

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.

[Sihui Liu]

Created attachment 436760 [details]
Patch

[Chris Dumez]

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?

[Sihui Liu]

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.

[Chris Dumez]

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.

[Chris Dumez]

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;

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 436784 [details]
Patch

[Chris Dumez]

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()).

[Sihui Liu]

Created attachment 436789 [details]
Patch

[EWS]

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].

[Chris Dumez]

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.