[Webkit-unassigned] [Bug 66477] [GTK] Html5 Notification, testing facilities

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66477


Philippe Normand <pnormand at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #112686|review?                     |review-
               Flag|                            |




--- Comment #6 from Philippe Normand <pnormand at igalia.com>  2011-11-03 08:24:28 PST ---
(From update of attachment 112686)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.