[Webkit-unassigned] [Bug 61140] [GTK][WK2] Add HTML5 Notifications support

[bugzilla-daemon at webkit.org]

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





--- Comment #77 from Martin Robinson <mrobinson at webkit.org>  2013-02-12 07:41:21 PST ---
(From update of attachment 187815)
View in context: https://bugs.webkit.org/attachment.cgi?id=187815&action=review

Looks pretty good to me. Carlos might have some more comments.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:76
> +//  , m_provider(this)

You can just remove this line.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:99
> +static void
> +notifyNotificationClosedCallback(NotifyNotification* notifyNotification, WebKitNotificationProvider* provider)

A Gnome-ism snuck in here.

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:113
> +    NotifyNotification* n = m_notifications.get(toImpl(notification)->notificationID());

Instead of "n", you should use a world like "notification."

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:138
> +void WebKitNotificationProvider::didDestroyNotification(WKNotificationRef notification)
> +{
> +}
> +
> +void WebKitNotificationProvider::clearNotifications(WKArrayRef notificationsIDs)
> +{
> +}

These could just be 0 in the struct right?

> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:30
> +#include <wtf/RefCounted.h>
> +
> +#include <libnotify/notify.h>

<libnotify> should come before <wtf> in the list. In WebKit, <wtf> isn't considered internal and all include go into the same clump. Yeah, I know it's uptight.

> Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:159
> +    GRefPtr<WebKitNotificationPermissionRequest> notificationPermissionRequest = adoptGRef(webkitNotificationPermissionRequestCreate(toImpl(request)));
> +    webkitWebViewMakePermissionRequest(WEBKIT_WEB_VIEW(clientInfo), WEBKIT_PERMISSION_REQUEST(notificationPermissionRequest.get()));

You probably want to update the documentation for permission-request to include a switch statement for the different types of permission requests. It should be blindingly obvious how to handle them all. For example, see: http://webkitgtk.org/reference/webkit2gtk/unstable/WebKitWebView.html#WebKitWebView-decide-policy

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:143
> +#if ENABLE(NOTIFICATIONS)
> +    RefPtr<WebKitNotificationProvider> notificationProvider;
> +#endif

It makes more sense here to use an OwnPtr and eliminate the RefCounted ancestry from WebKitNotificationProvider. RefPtr is good for shared objects, but since this object isn't shared it should be an OwnPtr. This isn't a huge problem, but the usage of a reference counted object can be misleading to those reading the code and make refactoring harder. It's also ever so slightly more efficient to avoid reference counting. I'm not sure why geolocationProvider is reference counted.

> Source/autotools/FindDependencies.m4:439
> +# check if libnotify is available

You can ditch this comment. I just finished nuking a bunch like this. :)

> Source/autotools/ReadCommandLineArguments.m4:132
> +AC_MSG_CHECKING([whether to enable notifications support])
> +AC_ARG_ENABLE(notifications,
> +              AC_HELP_STRING([--enable-notifications],
> +                             [enable support for notifications [default=yes]]),
> +              [],[enable_notifications="yes"])
> +AC_MSG_RESULT([$enable_notifications])
> +

I think it makes sense to enable this always for now and just expose a runtime setting. To me, geolocation is optional in part because geoclue is uncommon and because of the potential privacy violations.

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