[webkit-reviews] review denied: [Bug 66477] [GTK] Html5 Notification, testing facilities : [Attachment 112686] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Alexandre Mazari
<scaroo at gmail.com>'s request for review:
Bug 66477: [GTK] Html5 Notification, testing facilities
https://bugs.webkit.org/show_bug.cgi?id=66477

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

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
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.