Notification.requestPermission() should return a Promise

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Notification.requestPermission() should return a Promise.

[Chris Dumez]

Created attachment 428416 [details]
Patch

[Chris Dumez]

Created attachment 428418 [details]
Patch

[Chris Dumez]

Created attachment 428439 [details]
Patch

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 428440 [details]
Patch

[Chris Dumez]

Created attachment 428443 [details]
Patch

[Darin Adler]

Comment on attachment 428443 [details]
Patch

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

> Source/WebCore/Modules/notifications/NotificationClient.h:69
> +    virtual void requestPermission(ScriptExecutionContext*, CompletionHandler<void(NotificationClient::Permission)>&&) = 0;

This should take a ScriptExecutionContext&, not a pointer, since it’s required to be non-null.

> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:68
> +        for (auto& completionHandler : completionHandlers)
> +            completionHandler(NotificationClient::Permission::Denied);

Seems like we could use a helper function for this loop since it’s done twice.

> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:90
> +        for (auto& completionHandler : completionHandlers)
> +            completionHandler(allowed ? NotificationClient::Permission::Granted : NotificationClient::Permission::Denied);

Here is the second time that loop is done.

> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.h:52
> +    void startRequest(const WebCore::SecurityOriginData&, CompletionHandler<void(WebCore::NotificationClient::Permission)>&&);

A named type for the completion handler could possibly make the code less verbose.

> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.h:65
> +    HashMap<WebCore::SecurityOriginData, Vector<CompletionHandler<void(WebCore::NotificationClient::Permission)>>> m_requestsPerOrigin;

A named type for the vector could possibly make the code less verbose.

[Darin Adler]

Comment on attachment 428443 [details]
Patch

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

>> Source/WebCore/Modules/notifications/NotificationClient.h:69
>> +    virtual void requestPermission(ScriptExecutionContext*, CompletionHandler<void(NotificationClient::Permission)>&&) = 0;
> 
> This should take a ScriptExecutionContext&, not a pointer, since it’s required to be non-null.

Would really like you to do this one if you’re willing.

>> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:68
>> +            completionHandler(NotificationClient::Permission::Denied);
> 
> Seems like we could use a helper function for this loop since it’s done twice.

Not sure this is such a great idea; I think you’ll have to decide if it’s really helpful.

>> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.cpp:90
>> +            completionHandler(allowed ? NotificationClient::Permission::Granted : NotificationClient::Permission::Denied);
> 
> Here is the second time that loop is done.

Ditto.

>> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.h:52
>> +    void startRequest(const WebCore::SecurityOriginData&, CompletionHandler<void(WebCore::NotificationClient::Permission)>&&);
> 
> A named type for the completion handler could possibly make the code less verbose.

Ditto.

>> Source/WebKit/WebProcess/Notifications/NotificationPermissionRequestManager.h:65
>> +    HashMap<WebCore::SecurityOriginData, Vector<CompletionHandler<void(WebCore::NotificationClient::Permission)>>> m_requestsPerOrigin;
> 
> A named type for the vector could possibly make the code less verbose.

Ditto.

[Chris Dumez]

Created attachment 428456 [details]
Patch

[EWS]

Committed r277420 (237670@main): <https://commits.webkit.org/237670@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428456 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77951307>