Bug 95127 – [WK2] Add SPI for injected bundle to manually set permissions

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jon Lee]

TestRunner will need to be able to manually set permissions for notifications tests. We provide test-only SPI that allows direct manipulation of the cached permissions mapping in the web process.

Doing this instead of calling into the UI process to handle permissions makes tests legible. Otherwise, because of the IPC messaging involved, tests would require a cascade of setTimeout(nextStepInTestAfterMessageHasBeenDelivered, 0).

[Radar WebKit Bug Importer]

<rdar://problem/12182635>

[Jon Lee]

Created attachment 160855 [details]
Patch

[Jon Lee]

Created attachment 160862 [details]
Patch

[Jessie Berlin]

Comment on attachment 160862 [details]
Patch

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(originString, 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.cpp: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.

[Jon Lee]

Comment on attachment 160862 [details]
Patch

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.

In InjectedBundle.h there's a block of functions marked as TestRunner SPI, and I guess for whatever reason it's not like that here. I'll can rearrangement the declaration of the methods to provide a similar section in WKBundlePrivate.h.

>> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:541
>> +    page->notificationPermissionRequestManager()->setPermissionLevelForTesting(originString, 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.

No, using an enum means I would be hardcoding the origin strings in the library, and I'd rather have WTR/DRT be able to define it, in case it changes. (Right now http/tests uses 127.0.0.1:8000).
I'll make it consistent with "allowed".

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

Done.

>> Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp: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.

I'll split it out.

[Jon Lee]

Committed r126899: <http://trac.webkit.org/changeset/126899>