[webkit-reviews] review granted: [Bug 225712] Notification.requestPermission() should return a Promise : [Attachment 428443] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 225712: Notification.requestPermission() should return a Promise
https://bugs.webkit.org/show_bug.cgi?id=225712

Attachment 428443: Patch

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




--- Comment #7 from Darin Adler <darin at apple.com> ---
Comment on attachment 428443
  --> https://bugs.webkit.org/attachment.cgi?id=428443
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:5
2
> +    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:6
5
> +    HashMap<WebCore::SecurityOriginData,
Vector<CompletionHandler<void(WebCore::NotificationClient::Permission)>>>
m_requestsPerOrigin;

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