[webkit-reviews] review granted: [Bug 95127] [WK2] Add SPI for injected bundle to manually set permissions : [Attachment 160862] Patch

[bugzilla-daemon at webkit.org]

Jessie Berlin <jberlin at webkit.org> has granted Jon Lee <jonlee at apple.com>'s
request for review:
Bug 95127: [WK2] Add SPI for injected bundle to manually set permissions
https://bugs.webkit.org/show_bug.cgi?id=95127

Attachment 160862: Patch
https://bugs.webkit.org/attachment.cgi?id=160862&action=review

------- Additional Comments from Jessie Berlin <jberlin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=160862&action=review


> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePrivate.h:99
> +WK_EXPORT void WKBundleRemoveAllWebNotificationPermissions(WKBundleRef
bundle, WKBundlePageRef page);

You should add a comment here (or above the block of functions in this file
that are only used for testing) that this should only be used for testing.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:541
> +   
page->notificationPermissionRequestManager()->setPermissionLevelForTesting(orig
inString, allowed ? NotificationClient::PermissionAllowed :
NotificationClient::PermissionDenied);

Is the originString always a file URL or localhost since this is for the tests?
In which case, would it be better to pass an enum or bool value indicating
which of the two it is?

Why do you call the bool allowed here while you call in granted in
WKBundlePrivate.h and WKBundle.cpp? You should pick one and use it throughout.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.h:127
> +    void setWebNotificationPermission(WebPage*, const String&, bool);

You should include the parameter names of the string and the bool, since they
are not obvious.

>
Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cp
p:157
> +   
WebProcess::shared().notificationManager().didUpdateNotificationDecision(origin
->toString(), allowed);

Why is this being done in this patch? From your comment, it seems like this
needs to be done for notifications in general, not to enable testing. If so,
please split it out into a separate patch.