Bug 225712 - Notification.requestPermission() should return a Promise
Summary: Notification.requestPermission() should return a Promise
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 225711
Blocks:
  Show dependency treegraph
 
Reported: 2021-05-12 12:11 PDT by Chris Dumez
Modified: 2021-05-12 22:03 PDT (History)
12 users (show)

See Also:


Attachments
Patch (32.35 KB, patch)
2021-05-12 14:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (32.47 KB, patch)
2021-05-12 14:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (32.54 KB, patch)
2021-05-12 17:43 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (32.60 KB, patch)
2021-05-12 18:04 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (32.74 KB, patch)
2021-05-12 18:09 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (34.72 KB, patch)
2021-05-12 21:17 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-05-12 12:11:57 PDT
Notification.requestPermission() should return a Promise.
Comment 1 Chris Dumez 2021-05-12 14:04:45 PDT
Created attachment 428416 [details]
Patch
Comment 2 Chris Dumez 2021-05-12 14:14:59 PDT
Created attachment 428418 [details]
Patch
Comment 3 Chris Dumez 2021-05-12 17:43:22 PDT
Created attachment 428439 [details]
Patch
Comment 4 EWS Watchlist 2021-05-12 17:44:10 PDT
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 5 Chris Dumez 2021-05-12 18:04:59 PDT
Created attachment 428440 [details]
Patch
Comment 6 Chris Dumez 2021-05-12 18:09:28 PDT
Created attachment 428443 [details]
Patch
Comment 7 Darin Adler 2021-05-12 20:33:57 PDT
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.
Comment 8 Darin Adler 2021-05-12 20:39:40 PDT
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.
Comment 9 Chris Dumez 2021-05-12 21:17:44 PDT
Created attachment 428456 [details]
Patch
Comment 10 EWS 2021-05-12 22:02:52 PDT
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].
Comment 11 Radar WebKit Bug Importer 2021-05-12 22:03:16 PDT
<rdar://problem/77951307>