Bug 66477 - [GTK] Html5 Notification, testing facilities
Summary: [GTK] Html5 Notification, testing facilities
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 94498 (view as bug list)
Depends on: 61140 63616 71940
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-18 10:13 PDT by Alexandre Mazari
Modified: 2014-04-08 18:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (13.85 KB, patch)
2011-08-18 10:18 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (14.14 KB, patch)
2011-10-27 08:19 PDT, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2011-11-09 11:33 PST, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (11.90 KB, patch)
2011-11-09 11:36 PST, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2011-11-10 03:00 PST, Alexandre Mazari
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2011-11-10 03:49 PST, Alexandre Mazari
pnormand: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Mazari 2011-08-18 10:13:06 PDT
The attached patch provides the testing facilities to pass fast/notifications and http/test/notifications.
It also unskip the passing tests.
Comment 1 Alexandre Mazari 2011-08-18 10:18:30 PDT
Created attachment 104357 [details]
Patch
Comment 2 Alexandre Mazari 2011-08-19 01:08:08 PDT
(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 :)
Comment 3 Philippe Normand 2011-09-23 06:19:43 PDT
(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?
Comment 4 Alexandre Mazari 2011-10-27 08:19:34 PDT
Created attachment 112686 [details]
Patch
Comment 5 Philippe Normand 2011-11-02 07:57:21 PDT
(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 6 Philippe Normand 2011-11-03 08:24:28 PDT
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 7 Martin Robinson 2011-11-03 10:36:09 PDT
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.
Comment 8 Alexandre Mazari 2011-11-09 11:33:30 PST
Created attachment 114326 [details]
Patch
Comment 9 Alexandre Mazari 2011-11-09 11:36:11 PST
Created attachment 114329 [details]
Patch
Comment 10 Alexandre Mazari 2011-11-10 03:00:48 PST
Created attachment 114464 [details]
Patch
Comment 11 Alexandre Mazari 2011-11-10 03:05:04 PST
(In reply to comment #10)
> Created an attachment (id=114464) [details]
> Patch

Updated:

- Move back all testing code from DRTSupportGtk to DRT
Comment 12 Alexandre Mazari 2011-11-10 03:49:53 PST
Created attachment 114466 [details]
Patch
Comment 13 Alexandre Mazari 2011-11-10 03:51:20 PST
(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 14 Philippe Normand 2011-11-23 09:24:47 PST
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.
Comment 15 Martin Robinson 2012-08-20 21:38:33 PDT
*** Bug 94498 has been marked as a duplicate of this bug. ***
Comment 16 Martin Robinson 2014-04-08 18:48:50 PDT
The GTK+ port of WebKit1 has been removed.