Bug 95127

Summary: [WK2] Add SPI for injected bundle to manually set permissions
Product: WebKit Reporter: Jon Lee <jonlee>
Component: Tools / TestsAssignee: Jon Lee <jonlee>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, haraken, jberlin, jianli, philn, sam, webkit-bug-importer, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.8   
Bug Depends on:    
Bug Blocks: 77969    
Attachments:
Description Flags
Patch
none
Patch jberlin: review+

Description Jon Lee 2012-08-27 14:17:13 PDT
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).
Comment 1 Radar WebKit Bug Importer 2012-08-27 14:19:28 PDT
<rdar://problem/12182635>
Comment 2 Jon Lee 2012-08-27 16:44:44 PDT
Created attachment 160855 [details]
Patch
Comment 3 Jon Lee 2012-08-27 17:12:01 PDT
Created attachment 160862 [details]
Patch
Comment 4 Jessie Berlin 2012-08-28 09:27:58 PDT
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.
Comment 5 Jon Lee 2012-08-28 10:13:48 PDT
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.
Comment 6 Jon Lee 2012-08-28 11:11:23 PDT
Committed r126899: <http://trac.webkit.org/changeset/126899>