Bug 225712

Summary:

Notification.requestPermission() should return a Promise

Product:

WebKit

Reporter:

Chris Dumez <cdumez>

Component:

WebCore Misc.

Assignee:

Chris Dumez <cdumez>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

achristensen, clopez, darin, esprehn+autocc, ews-watchlist, ggaren, hi, kondapallykalyan, mkwst, sam, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

225711

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-
Patch	none
Patch	none
Patch	none

Chris Dumez

Reported 2021-05-12 12:11:57 PDT

Notification.requestPermission() should return a Promise.

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Chris Dumez

Comment 1 2021-05-12 14:04:45 PDT

Created attachment 428416 [details] Patch

Chris Dumez

Comment 2 2021-05-12 14:14:59 PDT

Created attachment 428418 [details] Patch

Chris Dumez

Comment 3 2021-05-12 17:43:22 PDT

Created attachment 428439 [details] Patch

EWS Watchlist

Comment 4 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

Chris Dumez

Comment 5 2021-05-12 18:04:59 PDT

Created attachment 428440 [details] Patch

Chris Dumez

Comment 6 2021-05-12 18:09:28 PDT

Created attachment 428443 [details] Patch

Darin Adler

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

Darin Adler

Comment 8 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.

Chris Dumez

Comment 9 2021-05-12 21:17:44 PDT

Created attachment 428456 [details] Patch

EWS

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

Radar WebKit Bug Importer

Comment 11 2021-05-12 22:03:16 PDT

<rdar://problem/77951307>

Note You need to log in before you can comment on or make changes to this bug.