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).
<rdar://problem/12182635>
Created attachment 160855 [details] Patch
Created attachment 160862 [details] Patch
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 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.
Committed r126899: <http://trac.webkit.org/changeset/126899>