Bug 66477

Created attachment 104357 [details] Patch

(In reply to comment #1) > Created an attachment (id=104357) [details] > Patch This patch add the required testing facilities to gtk DRT and LTC to pass notifications-related tests. The passing tests are also unskipped. The bulk of the code is located in Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk which from a conversation with mrobinson ain't really the right place. The rationals behind this choice are: - DRT and the javascript LTC api must both have access to the list of security requests and authorized orgins. - the notifications testing code must be conditionally included by the preprocessor depending on the ENABLE(NOTIFICATIONS) macro value so to not break the compilation when notifications are disabled. Sadly this macro is not working in Tools/ so I had to artificially move the code within the Source/WebKit/gtk tree. I am not satisfied with this solution so any idea appreciated :)

(In reply to comment #2) > - the notifications testing code must be conditionally included by the preprocessor depending on the ENABLE(NOTIFICATIONS) macro value so to not break the compilation when notifications are disabled. > > Sadly this macro is not working in Tools/ so I had to artificially move the code within the Source/WebKit/gtk tree. > That macro is defined in wtf/Platform.h which you should be able to include from Tools/DumpRenderTree, or am I missing something?

Created attachment 112686 [details] Patch

(In reply to comment #4) > Created an attachment (id=112686) [details] > Patch This patch doesn't seem to change a lot compared to the previous iteration? Also, I think the ENABLE macro not working in DRT is due to not including webcore_cppflags into Programs_DumpRenderTree_CPPFLAGS.

Comment on attachment 112686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112686&action=review > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:907 > + return 0; This can be removed :) > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:918 > +static void removeDesktopNotification(WebKitWebNotification* notification) > +{ > + s_notifications = g_list_remove(s_notifications, notification); > +} Why make a specific function for this? It's used in one place only. > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:944 > + gchar* notificationStr = webkit_web_notification_to_string(notification); > + printf("%s\n", notificationStr); > + g_free(notificationStr); Please use a GOwnPtr<gchar> like in other places. The explicit g_free call would then not be needed. > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:961 > + gchar* originStr = webkitSecurityOriginToString(origin); Ditto > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:976 > + gchar* originStr = webkitSecurityOriginToString(origin); Ditto > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:985 > + gchar* originStr = webkitSecurityOriginToString(origin); Ditto > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:1002 > + g_object_connect(G_OBJECT(view), > + "signal::notification-show", showNotification, 0, > + "signal::notification-cancel", cancelNotification, 0, > + "signal::notification-request-permission", requestNotificationPermission, 0, > + "signal::notification-cancel-permission-requests", cancelNotificationPermissionRequest, 0, > + "signal::notification-check-permission", checkNotificationPermission, 0, > + 0); I think this could be done directly in DumpRenderTree.cpp. The last argument of g_object_connect() should be NULL, not 0. Also, all the printf calls should be moved to DumpRenderTree.cpp, like it's done for the other features. This will involve some code refactoring but it's doable.

Comment on attachment 112686 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112686&action=review > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:1028 > +void DumpRenderTreeSupportGtk::simulateDesktopNotificationClick(char* title) > +{ > +#if ENABLE(NOTIFICATIONS) > + GList* match = g_list_find_custom(s_notifications, title, (GCompareFunc)matchNotificationByTitle); > + > + if (!match) > + return; > + > + WebKitWebNotification* notification = WEBKIT_WEB_NOTIFICATION(match->data); > + webkit_web_notification_clicked(notification); > +#endif > +} I think you could make this the only method in DumpRenderTreeSupport. It could take the WebKitWebNotification as the argument.

Created attachment 114326 [details] Patch

Created attachment 114329 [details] Patch

Created attachment 114464 [details] Patch

(In reply to comment #10) > Created an attachment (id=114464) [details] > Patch Updated: - Move back all testing code from DRTSupportGtk to DRT

Created attachment 114466 [details] Patch

(In reply to comment #12) > Created an attachment (id=114466) [details] > Patch Updated: - use GOwnPtr for local char* variable, removing the need to explicitely free them.

Comment on attachment 114466 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=114466&action=review Thanks! This looks quite good, I found some little nits to fix though :) > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1125 > + return g_strdup_printf("%s://%s", > + webkit_security_origin_get_protocol(origin), > + webkit_security_origin_get_host(origin)); This can fit in one or two lines. > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1176 > + JSStringRef jsOrigin = JSStringCreateWithUTF8CString(originStr.get()); Weird indentation :) > Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1197 > + bool permited = gLayoutTestController->checkDesktopNotificationPermission(jsOrigin); s/permited/allowed ? > Tools/DumpRenderTree/gtk/LayoutTestControllerGtk.cpp:1037 > + if (!match) notificationTitle is leaked in this case.

*** Bug 94498 has been marked as a duplicate of this bug. ***

The GTK+ port of WebKit1 has been removed.