Bug 61140 - [GTK][WK2] Add HTML5 Notifications support
: [GTK][WK2] Add HTML5 Notifications support
Status: NEW
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: Gtk, LayoutTestFailure
: 107934
: 40829 63615 66477
  Show dependency treegraph
 
Reported: 2011-05-19 13:36 PST by
Modified: 2013-10-22 00:58 PST (History)


Attachments
Patch (52.29 KB, patch)
2011-05-19 14:07 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (59.42 KB, patch)
2011-06-17 07:41 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (78.39 KB, patch)
2011-06-20 06:52 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (79.22 KB, patch)
2011-06-20 13:19 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (80.25 KB, patch)
2011-06-21 03:35 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (56.02 KB, patch)
2011-06-29 04:17 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.38 KB, patch)
2011-06-29 09:48 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.81 KB, patch)
2011-07-01 03:04 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (52.82 KB, patch)
2011-07-01 03:12 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (51.30 KB, patch)
2011-08-18 09:46 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (38.15 KB, patch)
2011-08-18 10:07 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.99 KB, patch)
2011-09-27 01:42 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.96 KB, patch)
2011-10-27 08:00 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.18 KB, patch)
2011-11-09 11:15 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.15 KB, patch)
2011-11-10 02:15 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.28 KB, patch)
2011-11-10 09:14 PST, Alexandre Mazari
no flags Review Patch | Details | Formatted Diff | Diff
Patch (37.24 KB, patch)
2011-11-14 05:58 PST, Alexandre Mazari
gns: review-
Review Patch | Details | Formatted Diff | Diff
Patch (30.92 KB, patch)
2013-02-11 08:53 PST, Claudio Saavedra
no flags Review Patch | Details | Formatted Diff | Diff
Patch (34.46 KB, patch)
2013-02-12 02:31 PST, Claudio Saavedra
csaavedra: review?
gtk-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-19 13:36:21 PST
[GTK] Add HTML5 Notifications support
------- Comment #1 From 2011-05-19 14:07:10 PST -------
Created an attachment (id=94111) [details]
Patch
------- Comment #2 From 2011-05-19 14:09:55 PST -------
Attachment 94111 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2011-05-19 15:47:43 PST -------
(From update of attachment 94111 [details])
Attachment 94111 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8719196
------- Comment #4 From 2011-05-23 11:34:30 PST -------
(From update of attachment 94111 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=94111&action=review

Nice work overall, though I have a few concerns. The API is a bit confusing and it has no documentation. Be sure to add documentation in your next version of the patch. Beforehand it would be nice to talk about how the API is used. Ideally it should be as similar to other ports as possible. Careful with code style. Sorry if it seems like some of the following review comments are overly picky!

> LayoutTests/platform/gtk/Skipped:263
> +# Notification support needs improvement in unexpected conditions
> +fast/notifications/notifications-document-close-crash.html
> +fast/notifications/notifications-cancel-request-permission.html
> +fast/notifications/notification-after-close.html
>  http/tests/notifications

Why are these failing still?

> Source/WebKit/gtk/ChangeLog:20
> +        * GNUmakefile.am:
> +        * WebCoreSupport/ChromeClientGtk.cpp:
> +        (WebKit::ChromeClient::notificationPresenter):
> +        * WebCoreSupport/ChromeClientGtk.h:
> +        * WebCoreSupport/DumpRenderTreeSupportGtk.cpp:
> +        (matchNotificationByTitle):
> +        (matchNotificationByReplaceId):
> +        (DumpRenderTreeSupportGtk::resetDesktopNotificationPermissions):
> +        (DumpRenderTreeSupportGtk::grantDesktopNotificationPermission):
> +        (DumpRenderTreeSupportGtk::checkDesktopNotificationPermission):
> +        (DumpRenderTreeSupportGtk::ignoreDesktopNotificationPermissionRequests):
> +        (DumpRenderTreeSupportGtk::areDesktopNotificationPermissionRequestsIgnored):
> +        (DumpRenderTreeSupportGtk::simulateDesktopNotificationClick):

Please fill out these descriptions.

> Source/WebKit/gtk/GNUmakefile.am:255
> +

This seems unecessary.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:81
> +GList* DumpRenderTreeSupportGtk::s_notificationsAllowedOrigins = 0;
> +GList* DumpRenderTreeSupportGtk::s_notifications = 0;

Instead of using class static members here, please simply keep them static inside the file. I recommend using WTF::Vector here and WebCore Strings.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:821
> +static gint matchNotificationByTitle(WebKitWebNotification* notification,
> +                                     char* title)

Typically in WebKitGTK+ code we do not put newlines in argument lists.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:823
> +    return g_strcmp0(webkit_web_notification_get_title(notification), title);

I think it might be clearer to remove this helper.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:830
> +    return g_strcmp0(webkit_web_notification_get_replaceid(notification1),
> +                     webkit_web_notification_get_replaceid(notification2));

Ditto.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:836
> +    g_list_free_full(s_notificationsAllowedOrigins, g_free);
> +    s_notificationsAllowedOrigins = 0;

This would be much simpler if you used WTF::Vector for instance.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:31
> +
> +

This seems accidental.

> Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:136
> +    static void replaceDesktopNotificationWith(WebKitWebNotification*);
> +    static void resetDesktopNotifications();
> +    static void addDesktopNotification(WebKitWebNotification*);
> +    static void removeDesktopNotification(WebKitWebNotification*);
> +    static void resetDesktopNotificationPermissions();
> +    static void grantDesktopNotificationPermission(char*);
> +    static bool checkDesktopNotificationPermission(char*);
> +    static void ignoreDesktopNotificationPermissionRequests(gboolean);
> +    static bool areDesktopNotificationPermissionRequestsIgnored();
> +    static void simulateDesktopNotificationClick(char* title);
> +

This is a lot of private functions. Should some of these go in the public API?

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:30
> +#include "CString.h"
> +#include "Event.h"
> +#include "HashMap.h"
> +#include "Notification.h"
> +#include "SecurityOrigin.h"
> +
> +#include "webkitwebnotification.h"
> +#include "webkitwebnotificationprivate.h"
> +

This should all be one block of includes.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:51
> +    WebKitWebNotification* gnotification = (WebKitWebNotification *) g_object_new(WEBKIT_TYPE_WEB_NOTIFICATION, NULL);

Please use a C++ style static_cast here. Typically we would call this something like kitNotification.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:52
> +    webkit_web_notification_set_real(gnotification, notification);

Wouldn't it be better to set this with g_object_new?

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:58
> +                          &isOk);

Would isOkay be more accurately named if it was called permissionGranted?

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:61
> +        event = Event::create("display", false, true);

Does Event::create return a PassRefPtr or a raw Event*. If it's the latter, this is a memory leak, since RefPtr takes a reference.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:75
> +    g_signal_emit_by_name(m_webView, "notification-cancel"
> +                          gnotification.get());

Typically we let lines continue to 120 characters before breaking them.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:78
> +    RefPtr<Event> event = Event::create(eventNames().closeEvent,
> +                                        false, true);

Ditto and throughout the patch.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:94
> +    gchar* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> +    g_signal_emit_by_name(m_webView, "notification-check-permission",
> +                          origin, &permission);
> +    g_free(origin);

Here you can just use context->securityOrigin()->toString().utf8().data() in the function call and avoid an entire string copy.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:103
> +    char* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> +    g_signal_emit_by_name(m_webView, "notification-request-permission", origin)
> +    g_free(origin);

Ditto.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:113
> +    char* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> +    g_signal_emit_by_name(m_webView, "notification-cancel-requests-for-permission", origin);
> +    g_free(origin);

Ditto.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:28
> +#include "GRefPtr.h"
> +#include "Notification.h"
> +#include "NotificationPresenter.h"
> +#include "ScriptExecutionContext.h"
> +
> +#include "webkitwebnotification.h"

This should all be one block of includes.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:36
> +    public:

The public / private / protected labels should not be indented per WebKit style.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:53
> +#endif /* NOTIFICATIONPRESENTERGTK_H_ */

Please use a C++ style comment here and fix the spelling of NOTIFICATIONPRESENTERGTK_H_ to be NotificationPresenterGtk_h.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:33
> +#include "Notification.h"
> +#include "UserGestureIndicator.h"
> +#include "webkitwebnotificationprivate.h"
> +
> +#include <glib.h>

Please make this all one block.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:35
> +G_DEFINE_TYPE (WebKitWebNotification, webkit_web_notification, G_TYPE_OBJECT);

No space after G_DEFINE_TYPE.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:38
> +    WebCore::Notification *notification;

Asterisks should be with the type name.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:43
> +    char* url;
> +    char* icon_url;
> +    char* title;
> +    char* body;
> +    char* dir;

Lately we've been making the private sections C++ objects which we call new and delete on manually (take a look at WebKitWebFrame, I believe -- or contact me via email for more information). If you did that you could change all these to be CString members and never have to worry about managing their memory.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:44
> +    char* replaceid;

replaceId should be replaceId, though perhaps just id would make sense?

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:132
> +    if (priv->dir)
> +        g_free(priv->dir);
> +    if (priv->body)
> +        g_free(priv->body);
> +    if (priv->title)
> +        g_free(priv->title);
> +    if (priv->url)
> +        g_free(priv->url);
> +    if (priv->icon_url)
> +        g_free(priv->icon_url);
> +    if (priv->replaceid)
> +        g_free(priv->replaceid);
> +

You could eliminate this completely for instance.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:136
> +static void
> +webkit_web_notification_class_init(WebKitWebNotificationClass* klass)

Please make this one line entirely and do the same everyehwere. There should also be a newline after the previous method's closing curly brace.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:147
> +    self->priv =
> +      G_TYPE_INSTANCE_GET_PRIVATE(self, WEBKIT_TYPE_WEB_NOTIFICATION,
> +                                  WebKitWebNotificationPrivate);

This can be one line.

> Source/WebKit/gtk/webkit/webkitwebnotification.h:61
> +#endif /* __WEBKIT_WEB_NOTIFICATION_H__ */

This seems like it should match the real guard name.

> Source/WebKit/gtk/webkit/webkitwebnotificationprivate.h:30
> +void webkit_web_notification_set_real (WebKitWebNotification *notification,
> +                                     WebCore::Notification *real);

This should be named with WebKit naming conventions since it's not a public header.

> Source/WebKit/gtk/webkit/webkitwebnotificationprivate.h:33
> +#endif /* __WEBKIT_WEB_NOTIFICATION_H__ */

This seems inaccurate.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2755
> +    webkit_web_view_signals[NOTIFICATION_SHOW] = g_signal_new("notification-show",

show-notification?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2757
> +            (GSignalFlags)(G_SIGNAL_RUN_LAST | G_SIGNAL_ACTION),

Please use a static_cast here and everywhere else.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2764
> +    webkit_web_view_signals[NOTIFICATION_CANCEL] = g_signal_new("notification-cancel",

cancel-notification?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2773
> +    webkit_web_view_signals[NOTIFICATION_REQUEST_PERMISSION] = g_signal_new("notification-request-permission",

request-notification-permission?

>> Source/WebKit/gtk/webkit/webkitwebview.h:85
>> +{
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

Please fix this.

> Tools/DumpRenderTree/LayoutTestController.cpp:2202
> +static JSValueRef ignoreDesktopNotificationPermissionRequestsCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)

Why is it necessary to change platform-independent code?

> Tools/DumpRenderTree/LayoutTestController.cpp:2210
> +
> +
> +static JSValueRef checkDesktopNotificationPermissionCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)

Too many newlines here.

> Tools/DumpRenderTree/LayoutTestController.cpp:2510
> -void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> -{
> -    m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> -}
> -
> -bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> -{
> -    std::vector<JSStringRef>::iterator i;
> -    for (i = m_desktopNotificationAllowedOrigins.begin();
> -         i != m_desktopNotificationAllowedOrigins.end();
> -         ++i) {
> -        if (JSStringIsEqual(*i, origin))
> -            return true;
> -    }
> -    return false;
> -}
> +// void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> +// {
> +//     m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> +// }
> +
> +// bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> +// {
> +//     std::vector<JSStringRef>::iterator i;
> +//     for (i = m_desktopNotificationAllowedOrigins.begin();
> +//          i != m_desktopNotificationAllowedOrigins.end();
> +//          ++i) {
> +//         if (JSStringIsEqual(*i, origin))
> +//             return true;
> +//     }
> +//     return false;
> +// }
>  

I don't understand the purpose of this change. In general, patches shouldn't retain code commented out. It worries me that this code is commented out in platfrom-independent code though. This will likely break other ports.

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1081
> +gboolean showNotification(WebKitWebView* webView, 
> +                          WebKitWebNotification* notification)

Please make this one line.
------- Comment #5 From 2011-05-23 15:10:12 PST -------
Hi Martin,

Thanks for the review. I prematurely posted it to here to get this kind of feedbacks validating/dismissing the global approach and regarding code-style and convention I am still not familiar with :)

(In reply to comment #4)
> (From update of attachment 94111 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94111&action=review
> 
> Nice work overall, though I have a few concerns. The API is a bit confusing and it has no documentation. Be sure to add documentation in your next version of the patch.

Yup I'll document the public API (mostly the signals) in a following iteration.

 Beforehand it would be nice to talk about how the API is used. Ideally it should be as similar to other ports as possible.

I tried to keep as close as possible to the Chrome an Qt API, themselves straight interpretations of the draft specification.

Here is basically the rules this patch tries to implement:
- js code request permission to show notifications from the current security domain using webkitNotifications.requestPermission()
- webkit-gtk, delegating to our NotificationPresenter impl, notifies the UA code using the "notification-request-permission" signal
- the UA ask the user and store its answers. Each security domain can have 3 states: allowed, declined and pending.
- when js instanciates a new notification, it checks implicitly the permission for the current security domain using NotificationPresenter::checkPermission
- this, in turn, delegates to the UA by sending the "notification-check-permission".
- if the UA forbid it, a security exception is thrown in the js context
- else the notification is now usable
- js code request the displaying of the notification
- we wrap the webkit notification in a GObject, and provide it to the UA by the mean of the "notification-show" signal
- the UA can decide to show it straight away or enqueue the notification for later use.
- at *actual* show-time, the "show" event is thrown in the js context (current implementation do not respect that, the "show" event is thrown at gsignal handlers return)
- in the case the notification couldnt be displayed, an "error" js event is fired.
- the js code can cancel the notification. If it is already show, the UA must hide it, if not, the notification must not be shown.
- if the system supports it, any click event on it must be forwarded to the js context. The proposed implementation provide the webkit_web_notification_clicked method for that purpose.
- webcore notifies us (callingceaNotificationPresenter::notificqtionObjectDestroyed when a kit'Notification object is dying. We use this oportunity to unref our GObject representation.
- A notification can replace a previous one. For that, its "replaceId" and security origin should equal a previously sent notification.


On a broader POV, I used signals to give control to the UA code in order to follow the surrounding conventions.
Though, in most cases, if not all, only one handler will be attached to each signal (1-1 rel). GObject signals are not type-safe, implie some overhead and do not enforce the webkit-gtk and client code to be logically segmented. See both WebKitWebView and EphyWebView for instance: huge inflation of signals/signals handlers.
I was wondering if your share this point of view and if in this case a more traditional delegation architecture, using an interface to be implemented by client and a setter in WebView would be fine to push at least for this feature.


>Careful with code style. Sorry if it seems like some of the following review comments are overly picky!
No pun taken, thanks again for the detailed review. Following a comment for each of your points that need precision. The left-outs do not require detailed explanation and will be corrected.

> 
> > LayoutTests/platform/gtk/Skipped:263
> > +# Notification support needs improvement in unexpected conditions
> > +fast/notifications/notifications-document-close-crash.html
> > +fast/notifications/notifications-cancel-request-permission.html
> > +fast/notifications/notification-after-close.html
> >  http/tests/notifications
> 
> Why are these failing still?
> 

Working on it, still one to go. Some of the tests seems to contradict with each-other and the draft spec.

> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:81
> > +GList* DumpRenderTreeSupportGtk::s_notificationsAllowedOrigins = 0;
> > +GList* DumpRenderTreeSupportGtk::s_notifications = 0;
> 
> Instead of using class static members here, please simply keep them static inside the file. I recommend using WTF::Vector here and WebCore Strings.

Ok. How do I define the comparator func so to compare on string content instead of address ?

> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:821
> > +static gint matchNotificationByTitle(WebKitWebNotification* notification,
> > +                                     char* title)
> 
> Typically in WebKitGTK+ code we do not put newlines in argument lists.

Ok

> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:823
> > +    return g_strcmp0(webkit_web_notification_get_title(notification), title);
> 
> I think it might be clearer to remove this helper.

Needed for the GList implementation, hopefully using Vector<String> will remove the need for them.

> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:830
> > +    return g_strcmp0(webkit_web_notification_get_replaceid(notification1),
> > +                     webkit_web_notification_get_replaceid(notification2));
> 
> Ditto.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:836
> > +    g_list_free_full(s_notificationsAllowedOrigins, g_free);
> > +    s_notificationsAllowedOrigins = 0;
> 
> This would be much simpler if you used WTF::Vector for instance.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:31
> > +
> > +
> 
> This seems accidental.
> 
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:136
> > +    static void replaceDesktopNotificationWith(WebKitWebNotification*);
> > +    static void resetDesktopNotifications();
> > +    static void addDesktopNotification(WebKitWebNotification*);
> > +    static void removeDesktopNotification(WebKitWebNotification*);
> > +    static void resetDesktopNotificationPermissions();
> > +    static void grantDesktopNotificationPermission(char*);
> > +    static bool checkDesktopNotificationPermission(char*);
> > +    static void ignoreDesktopNotificationPermissionRequests(gboolean);
> > +    static bool areDesktopNotificationPermissionRequestsIgnored();
> > +    static void simulateDesktopNotificationClick(char* title);
> > +
> 
> This is a lot of private functions. Should some of these go in the public API?

Well this a tricky one: most of those methods are exposed by the *custom* Qt and Chromium LayoutTesControllers. Those two do not implement the common LTC that do not declare those APIs.
So I added them to the common LHT and made LayoutTestControllerGtk implementations delegates to DumpRenderTreeControllerGtk because it is AFAIK the only place where state can be shared between DRT and LTC and DRT needs to access some of the state (the list of authorized security origins, the ignore flags...).

> 
> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:52
> > +    webkit_web_notification_set_real(gnotification, notification);
> 
> Wouldn't it be better to set this with g_object_new?
> 

I used a private setter to forbid the client code to touch it. Might better use a constructor only prop instead indeed.

> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:58
> > +                          &isOk);
> 
> Would isOkay be more accurately named if it was called permissionGranted?
> 

Yup

> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:61
> > +        event = Event::create("display", false, true);
> 
> Does Event::create return a PassRefPtr or a raw Event*. If it's the latter, this is a memory leak, since RefPtr takes a reference.
> 

Gotta check and ping you to explain me all the subtlety behind that.


> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:94
> > +    gchar* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> > +    g_signal_emit_by_name(m_webView, "notification-check-permission",
> > +                          origin, &permission);
> > +    g_free(origin);
> 
> Here you can just use context->securityOrigin()->toString().utf8().data() in the function call and avoid an entire string copy

Tried that initially but somehow the address was rewritten sometime; thus the copy. Maybe the JS/GC thread do its magic backstage.

> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:43
> > +    char* url;
> > +    char* icon_url;
> > +    char* title;
> > +    char* body;
> > +    char* dir;
> 
> Lately we've been making the private sections C++ objects which we call new and delete on manually (take a look at WebKitWebFrame, I believe -- or contact me via email for more information). If you did that you could change all these to be CString members and never have to worry about managing their memory.
> 

A'ight.

> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:44
> > +    char* replaceid;
> 
> replaceId should be replaceId, though perhaps just id would make sense?
> 

Well, the spec specifies the name replace id and this field is optional so 'id' might not convey the full intent.



> 
> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2755
> > +    webkit_web_view_signals[NOTIFICATION_SHOW] = g_signal_new("notification-show",
> 
> show-notification?

As you wish. I wanted to prefix the notification-related signals to "namespace them a bit.

> >> Source/WebKit/gtk/webkit/webkitwebview.h:85
> >> +{
> > 
> > This { should be at the end of the previous line  [whitespace/braces] [4]
> 
> Please fix this.
> 

The enums around do not put opening brace on the same line, so should I ?
BTW, do you think this enum should be located here or in webkiwebnotification.h ? I'd favor the 2nd solution.

> > Tools/DumpRenderTree/LayoutTestController.cpp:2202
> > +static JSValueRef ignoreDesktopNotificationPermissionRequestsCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
> 
> Why is it necessary to change platform-independent code?

See above.

> > Tools/DumpRenderTree/LayoutTestController.cpp:2510
> > -void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> > -{
> > -    m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> > -}
> > -
> > -bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> > -{
> > -    std::vector<JSStringRef>::iterator i;
> > -    for (i = m_desktopNotificationAllowedOrigins.begin();
> > -         i != m_desktopNotificationAllowedOrigins.end();
> > -         ++i) {
> > -        if (JSStringIsEqual(*i, origin))
> > -            return true;
> > -    }
> > -    return false;
> > -}
> > +// void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> > +// {
> > +//     m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> > +// }
> > +
> > +// bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> > +// {
> > +//     std::vector<JSStringRef>::iterator i;
> > +//     for (i = m_desktopNotificationAllowedOrigins.begin();
> > +//          i != m_desktopNotificationAllowedOrigins.end();
> > +//          ++i) {
> > +//         if (JSStringIsEqual(*i, origin))
> > +//             return true;
> > +//     }
> > +//     return false;
> > +// }
> >  
> 
> I don't understand the purpose of this change. In general, patches shouldn't retain code commented out. It worries me that this code is commented out in platfrom-independent code though. This will likely break other ports.

See above. this code was not used by any port AFAIK. In later patch, I moved the implementation within the *Gtk and added empty methods in other ports (wx, windows, mac).
Still we could reuse those impl, but thqt would imply a gchar* -> JSStringRef conversion each time we check for a permission.
------- Comment #6 From 2011-05-31 08:36:52 PST -------
(In reply to comment #5)

> I was wondering if your share this point of view and if in this case a more traditional delegation architecture, using an interface to be implemented by client and a setter in WebView would be fine to push at least for this feature.

While that is the optimal approach in some ways, and we are using it for certain features soon, it pushes some burden onto the embedder. Implementing an interface typically involves writing lots of GObject boilerplate, which is a significantly  higher barrier than just adding a signal handler. 


> Ok. How do I define the comparator func so to compare on string content instead of address ?

If you Make a Vector<String> comparisons will automatically work. You should just be able to use .find(...) or whatever method exists on Vector. I believe it should just use the contained classes operator== overload.

> Gotta check and ping you to explain me all the subtlety behind that.

Sure, feel free.

> > > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:94
> > > +    gchar* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> > > +    g_signal_emit_by_name(m_webView, "notification-check-permission",
> > > +                          origin, &permission);
> > > +    g_free(origin);
> > 
> > Here you can just use context->securityOrigin()->toString().utf8().data() in the function call and avoid an entire string copy

> Tried that initially but somehow the address was rewritten sometime; thus the copy. Maybe the JS/GC thread do its magic backstage.

Hrm. As long as the signal is actually emitted during the function call g_signal_emit_by_name it should be fine. If you want to save a copy in the handler though, you'll need to do an actual memory copy.

> > > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:44
> > > +    char* replaceid;
> > replaceId should be replaceId, though perhaps just id would make sense?
> Well, the spec specifies the name replace id and this field is optional so 'id' might not convey the full intent.

Okay. replaceId is probably better then.

> > >> Source/WebKit/gtk/webkit/webkitwebview.h:85
> > >> +{
> > > 
> > > This { should be at the end of the previous line  [whitespace/braces] [4]
> > 
> > Please fix this.
> The enums around do not put opening brace on the same line, so should I ?
> BTW, do you think this enum should be located here or in webkiwebnotification.h ? I'd favor the 2nd solution.

Sorry. This is a public header so you should ignore this style error. We should fix the style bot to ignore these headers.

> See above. this code was not used by any port AFAIK. In later patch, I moved the implementation within the *Gtk and added empty methods in other ports (wx, windows, mac).
> Still we could reuse those impl, but thqt would imply a gchar* -> JSStringRef conversion each time we check for a permission.

For these sorts of changes, it'd be good if you could ping the original author to have a look as well.
------- Comment #7 From 2011-06-17 07:41:21 PST -------
Created an attachment (id=97599) [details]
Patch
------- Comment #8 From 2011-06-17 07:43:45 PST -------
Attachment 97599 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #9 From 2011-06-17 07:56:32 PST -------
(In reply to comment #7)
> Created an attachment (id=97599) [details] [details]
> Patch

News in this patch:
- Hopefully fix any style issue reported by martin
- Use a C++ object as private struct for WebKitWebNotification
- Match specifications regarding event propagation (do it at actual display time). Useful in the case of notifications queuing by the client.
- Send WebKitSecurityOrigin instances instead of strings for permission requests.
- Public API (signals and methods) documentation
- minimize existing common code modification by reusing those facilities
- Pass all tests but one

Wanted reviews:
- API design
- code style
- memory management (references etc...)

Open questions:
- should cancelling permission request for a specific security origin only remove pending requests or also accepted (by the user) one ?

- should we make html notification handling transparent to the API user ?
(Misnamed) HTML notifications require their content (title, body, icon) be fetched from a provided url. Should that be the client responsibility ?

- again, do we want to add even more signals to WebKitWebView instead of using proper delegation ?


Thanks for your time !
------- Comment #10 From 2011-06-17 07:58:45 PST -------
(From update of attachment 97599 [details])
Attachment 97599 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8881560
------- Comment #11 From 2011-06-20 06:52:30 PST -------
Created an attachment (id=97796) [details]
Patch
------- Comment #12 From 2011-06-20 06:55:28 PST -------
Attachment 97796 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Tools/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #13 From 2011-06-20 07:21:49 PST -------
(From update of attachment 97796 [details])
Attachment 97796 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8907596
------- Comment #14 From 2011-06-20 13:19:14 PST -------
Created an attachment (id=97847) [details]
Patch
------- Comment #15 From 2011-06-21 03:35:45 PST -------
Created an attachment (id=97957) [details]
Patch
------- Comment #16 From 2011-06-21 03:37:23 PST -------
(In reply to comment #15)
> Created an attachment (id=97957) [details] [details]
> Patch

- Added documentation for WebKitWebNotificationPresenter interface
- remove unused code
- clean copyright headers
------- Comment #17 From 2011-06-21 05:48:57 PST -------
(From update of attachment 97957 [details])
Attachment 97957 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8909952
------- Comment #18 From 2011-06-21 11:27:56 PST -------
OK, after reading a bit about this, this is what I think would be a reasonable design for it:

- The permission bits would go in the WebView, as signals, to keep things consistent with what we have.

- There's one extra signal when the WebView wants to show a particular notification. The UA can connect to this and do whatever it wants with it.

- We have a notification object with all the data bits and two signals, show and cancel. This is the DOM object defined in the spec and available in our GObject bindings if we enable it (I don't think we are doing this now).

Also all the mentions to JavaScript in the doc are wrong, this is not directly related to JS other than JS being the default language to access the DOM from browsers.

This is mostly from reading http://www.w3.org/TR/notifications/#bib-PERMISSIONS and the suggested patch.
------- Comment #19 From 2011-06-21 11:34:21 PST -------
(In reply to comment #18)
> This is mostly from reading http://www.w3.org/TR/notifications/#bib-PERMISSIONS and the suggested patch.

Er, I meant simply http://www.w3.org/TR/notifications of course, not the PERMISSIONS bit.
------- Comment #20 From 2011-06-21 12:05:15 PST -------
(From update of attachment 97957 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=97957&action=review

I think that using interfaces will greatly complicate using this for no real good reason. I mostly agree with Xan: I'd rather have policy decisions go through WebView like we do for the rest, and I am pretty sure WebKitWebNotification is duplicating the DOM object, so enabling it sounds like a good first step =).

> Source/WebCore/ChangeLog:10
> +        * notifications/Notification.cpp: Use the same codepath as Qt, set the stqte before showing.

stqte->state

> Source/WebKit/gtk/ChangeLog:18
> +        on Gtk's NotificationPresenter implementation<

s/<//
------- Comment #21 From 2011-06-22 00:00:26 PST -------
(In reply to comment #18)
> OK, after reading a bit about this, this is what I think would be a reasonable design for it:
> 
> - The permission bits would go in the WebView, as signals, to keep things consistent with what we have.
> 
> - There's one extra signal when the WebView wants to show a particular notification. The UA can connect to this and do whatever it wants with it.
> 
I am really not sure signals are the best tools here: there  will always be only one interested party that will share state across calls. Class are made for that purpose by giving type safety and separation of concerns.
WebKitWebView and the mirroring EphyWebView are alreadyd kinda bloated IMHO with signals/handlers with unrelated concerns and the respective state. 

> - We have a notification object with all the data bits and two signals, show and cancel. This is the DOM object defined in the spec and available in our GObject bindings if we enable it (I don't think we are doing this now).
> 

Yeah reusing thate would be great, but the generated WebKitDOMNotification do not have fields/properties for the actual notification content (title, body and icon url). Somehow the IDL (Source/WebCore/notifications/Notification.idl) do not declare them.

> Also all the mentions to JavaScript in the doc are wrong, this is not directly related to JS other than JS being the default language to access the DOM from browsers.

You are right.

> This is mostly from reading http://www.w3.org/TR/notifications/#bib-PERMISSIONS and the suggested patch.

Thanks for the throughout review !
------- Comment #22 From 2011-06-22 00:51:16 PST -------
(In reply to comment #20)

Hi !

> I think that using interfaces will greatly complicate using this for no real >good reason.

Well, I dont really buy this argument, on webkit part, signals imply the generation of new marshaller, modifies existing code (with eventual side-effects) and makes WebKitWebView source even longer to human parse and test.

The same apply on client code, requiring interface implementation enforce data+behaviour locality, making it easier to share state between calls, finding your way to the related code and write unit tests for isolated components.

C/GObject interface adds some ceremonial for sure, but the longer term advantage regarding maintenance overshadows this initial step. Cumbersome to write, nice to maintain.

Also this is greatly mitigated when the client code uses language bindings with true object orientation.

Making the (nice and well-thought) object-oriented model of WebKit/WebCore a procedural one by flattening the type hierarchy souds like a step backward IMHO.

Shouldn't we strive to make the devs' (both UA and webkit-gtk contributors) life easier by:

- increasing their productivity by
  * avoiding them getting lost in 500+ loc sources
  * wasting time trying to find the reason their signals handler is not
    called, often a bloody typo when the compiler can do it better.
  * avoiding creating temp structures to pass data around multiple invocations
    and handling their reffing rightly.

- making them more confident in their code by
  * making building the security harness that are unit tests easier
  * avoiding in-place modifications which might introduce side-effects.

Oh this s turning into a OO manifesto, I d better stop that before a functional programmer comes chime in ;)

But one a more general aspect, I kinda think that the complexity and time-consuming nature of the GObject type system (in C) caused an abuse of signals usage throughout the platform (gnome), favouring a procedural paradigm for application development. Still signals are a great tools for watching unexpected hardware, network and user events when multiple parties are interested in. Outside that scope delegation is leaner, faster and more robust IMHO.

> I mostly agree with Xan: I'd rather have policy decisions go through WebView like we do for the rest,

Consistency sounds like a convincing argument. If the two of you really feel that way, I'll go back to the signal implementation for consistency-sake.

But in the longer term do we want to keep this status quo ?

> and I am pretty sure WebKitWebNotification is duplicating the DOM object, so enabling it sounds like a good first step =).
>

See the previous comment, the dom notification lakes essential data. Maybe I should subclass it and add what's missing.

> > Source/WebCore/ChangeLog:10
> > +        * notifications/Notification.cpp: Use the same codepath as Qt, set the stqte before showing.
> 
> stqte->state
>
Will Fix. 

> > Source/WebKit/gtk/ChangeLog:18
> > +        on Gtk's NotificationPresenter implementation<
> 
> s/<//
Itoo.

Thanks for your time and effort!
------- Comment #23 From 2011-06-22 04:12:11 PST -------
(In reply to comment #21)
> I am really not sure signals are the best tools here: there  will always be only one interested party that will share state across calls. Class are made for that purpose by giving type safety and separation of concerns.
> WebKitWebView and the mirroring EphyWebView are alreadyd kinda bloated IMHO with signals/handlers with unrelated concerns and the respective state. 

This is a false dichotomy. If you really feel it's better you can create a new object (WebKitWebNotificationCenter) that will be in charge of all notification related stuff. Then one would get that from the WebView and use it through signals, as we usually do. Whether we put them in WebView or not is a matter of convenience for the user. Pretending that signals force upon us bad design is disingenuous IMHO.

Once that is gone we are left with concerns about type safety and whatnot, which although valid I think are nowhere near as important as being consistent with the platform we target, which uses signals thoroughly, or any other number of concerns. Not to mention, of course, the multiple features signals provide, like multiplexing or chaining, which are often useful and sometimes a must-have. Signals have their uses, interfaces have their uses, we should use each one when it's appropriate to do so.

In any case I do not want this to derive into a metaphysical debate about OO design. This library is 3+ years old, and the platform we live in more than a decade old. We are not going to radically change how we structure libraries at this point, so let's get on with our work.

> 
> > - We have a notification object with all the data bits and two signals, show and cancel. This is the DOM object defined in the spec and available in our GObject bindings if we enable it (I don't think we are doing this now).
> > 
> 
> Yeah reusing thate would be great, but the generated WebKitDOMNotification do not have fields/properties for the actual notification content (title, body and icon url). Somehow the IDL (Source/WebCore/notifications/Notification.idl) do not declare them.
> 

You are right! Since this is actually an interface, deriving from EventTarget, I guess the right thing to do is to just add the interface to our bindings (see how EventTarget is done in WebCore/bindings/gobject) and create a new object ourselves that implements that as far as it's useful. For instance I think the right thing to do here would be to use the show/cancel functionality through standard add_event_listener methods in the notification object, since that's how it's meant to work apparently.
------- Comment #24 From 2011-06-29 04:17:49 PST -------
Created an attachment (id=99070) [details]
Patch
------- Comment #25 From 2011-06-29 05:36:48 PST -------
(From update of attachment 99070 [details])
Attachment 99070 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8965021
------- Comment #26 From 2011-06-29 09:48:10 PST -------
Created an attachment (id=99106) [details]
Patch
------- Comment #27 From 2011-06-29 09:49:17 PST -------
(In reply to comment #26)
> Created an attachment (id=99106) [details] [details]
> Patch

Going back to the signals implementations as requested.
------- Comment #28 From 2011-06-29 09:51:08 PST -------
Attachment 99106 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/gtk/Skipped', u'Sourc..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #29 From 2011-06-29 10:17:35 PST -------
(From update of attachment 99106 [details])
Attachment 99106 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8896013
------- Comment #30 From 2011-07-01 03:04:31 PST -------
Created an attachment (id=99450) [details]
Patch

Added ChangeLog entry
------- Comment #31 From 2011-07-01 03:06:47 PST -------
Attachment 99450 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/gtk/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/webkit/webkitwebview.h:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #32 From 2011-07-01 03:12:13 PST -------
Created an attachment (id=99453) [details]
Patch

Fix style error in Source/WebKit/gtk/ChangeLog
------- Comment #33 From 2011-07-01 03:13:57 PST -------
Attachment 99453 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:86:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #34 From 2011-07-01 05:18:20 PST -------
(From update of attachment 99453 [details])
Attachment 99453 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8970424
------- Comment #35 From 2011-07-07 08:55:06 PST -------
(In reply to comment #23)
> This is a false dichotomy. If you really feel it's better you can create a new object (WebKitWebNotificationCenter) that will be in charge of all notification related stuff. Then one would get that from the WebView and use it through signals, as we usually do. Whether we put them in WebView or not is a matter of convenience for the user. Pretending that signals force upon us bad design is disingenuous IMHO.
> 

An example of this is WebKitWebInspector, btw, which is an object hosted by the WebView centralizing all of the responsibility for the web inspector.
------- Comment #36 From 2011-07-07 09:32:23 PST -------
(From update of attachment 99453 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=99453&action=review

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:144
> +static HashMap<KURL, GdkPixbuf* > pixbuf_cache;

This seems to only be used by this function? It would be better to make it be part of its scope, I believe.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:147
> +    g_assert(webkit_web_notification_is_html(self) == FALSE);

Any reason not to use ASSERT()? It should say !webkit_web_notification_is_html(self) instead of doing == FALSE.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:156
> +        SharedBuffer* icon_buffer = self->priv->coreNotification->iconData();
> +        GInputStream* icon_stream = g_memory_input_stream_new_from_data(icon_buffer->data(), icon_buffer->size(), NULL);
> +
> +        icon_pixbuf = gdk_pixbuf_new_from_stream(icon_stream, NULL, NULL);
> +        g_object_unref(icon_stream);

This is a bit awkward, how about using WebCore::Image::getGdkPixbuf? This way we also guarantee that the WebKit loaders are used. Also, how do you prune the pixbuf cache to avoid it growing infinitely?

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:162
> +}
> +/**

Missing newline.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:164
> + *

Missing @self here.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:169
> +const char* webkit_web_notification_get_url(WebKitWebNotification* self)

We standardized on 'uri' inside WebKitGTK+ instead of url.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:234
> + * webkit_web_notification_get_replaceid:

replaceid is a bit awkward, should we call it replace_id?
------- Comment #37 From 2011-08-18 09:46:43 PST -------
Created an attachment (id=104352) [details]
Patch
------- Comment #38 From 2011-08-18 09:49:09 PST -------
Attachment 104352 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #39 From 2011-08-18 10:07:03 PST -------
Created an attachment (id=104354) [details]
Patch
------- Comment #40 From 2011-08-18 10:08:47 PST -------
Attachment 104354 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #41 From 2011-08-19 00:56:48 PST -------
(In reply to comment #39)
> Created an attachment (id=104354) [details] [details]
> Patch

So after a long hiatus, and a lot of going back and forth, here is an updated patch to introduce the html5 notifications support.
This patch now only contains the actual implementation of the spec.
The testing facilities (modifications of DRT, DRTSupport and LTC) were split in another patch, that might need some deeper review.

Updated since last patch:
- WebKitWebNotifications now inherits from the generated WebKitDOMNotification
- public api now exported using the WEBKIT_API macro
- the missing references to @self were added to the documentation
- removed the GdkPixbuf cache for the icons
- usage of Image::getGdkPixbuf
- Made _get_direction returns a GtkTextDirection
- added convenience api for _to_string, _equals and _replaces
- hopefully fixed style issues
- use ASSERT instead of g_assert
- rename _url to _uri
------- Comment #42 From 2011-09-23 06:12:43 PST -------
(From update of attachment 104354 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=104354&action=review

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:777
> +NotificationPresenter* ChromeClient::notificationPresenter() const

If this method creates a Presenter it should be named something like createNotificationPresenter() and probably pass its reference to the caller, I think, like createPopupMenu() does for instance.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:31
> +#if ENABLE(NOTIFICATIONS)

This could just go before the includes

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:79
> +        g_error ("%s", error->message);
> +        g_error_free (error);

No space before ( please :)

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:88
> + * notification. Internally, it propagate an "error" event in the DOM

s/propagate/propagates

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:103
> + * notification. Internally, it propagate a "display" event in the DOM

Ditto

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:118
> + * been clicked. Internally, it propagate a "click" event in the DOM

Ditto

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:177
> +    static HashMap<KURL, GdkPixbuf* > pixbuf_cache;
> +
> +    KURL icon_uri = core(WEBKIT_DOM_NOTIFICATION(self))->iconURL();

These variables are unused it seems.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:254
> + * Returns: whether @self replaces (as defined in the Notifications specs)

Replace what?

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:255
> + * other.

other... notification I guess!

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:290
> +    return ((notification == notification2)
> +            || (coreNotification1 == coreNotification2)

Hum is it really necessary to compare both the WebKitWebNotifications and Notifications? I feel like one comparison would be enough here.

> Source/WebKit/gtk/webkit/webkitwebnotificationprivate.h:4
> + * webkit-web-notification.h - Header for WebKitWebNotification
> + *
> + * Copyright (C) 2010 Igalia S.L.

header name and copyright year are wrong

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2769
> +     * For each notification, one of WebKitNotification:displayed or
> +     * WebKitNotification:failed *MUST* be called at actual display-time so

Missing : ?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2795
> +     * remove it from the display; if it has not yet be displayed,

s/be/been

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2796
> +     * the user agent must prevent its being displayed.

"must prevent its being displayed" is odd, do you mean "must prevent its display" ?
------- Comment #43 From 2011-09-27 01:42:57 PST -------
Created an attachment (id=108810) [details]
Patch
------- Comment #44 From 2011-09-27 01:44:29 PST -------
Attachment 108810 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #45 From 2011-09-27 09:41:51 PST -------
(From update of attachment 108810 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=108810&action=review

Apart from these issues and the one spotted by the style-checker this patch looks OK to me but we need another reviewer approval for the added API.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:31
> +#include "GRefPtr.h"
> +#include "Notification.h"
> +#include "NotificationPresenter.h"
> +#include "ScriptExecutionContext.h"
> +#include "webkitwebnotification.h"
> +
> +typedef struct _WebKitWebView WebKitWebView;
> +
> +#if ENABLE(NOTIFICATIONS)

Can you move the #if before the includes as I suggested in the previous review? Thanks!

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
> +    return ((notification == notification2)
> +            || (coreNotification1 == coreNotification2)

As I was asking in the previous review, is this superfluous?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2767
> +     * For each notification, one of WebKitNotification:displayed or

Missing ":"... WebKitNotification::displayed
------- Comment #46 From 2011-10-03 09:23:06 PST -------
(From update of attachment 108810 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=108810&action=review

Other than that name (replaces), the patch looks fine to me. I was feeling a need to bikeshed on the signal names (suggest they follow our usual naming scheme that uses past tenses), but I managed to control myself and think they're fine as they are.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:163
> + * webkit_web_notification_get_icon_uri:

s/_uri//

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:255
> +gboolean webkit_web_notification_replaces(WebKitWebNotification* self, WebKitWebNotification* other)

I think this name is misleading. Look at it this way:

self.replaces(other)

It would lead me to believe that we are checking if self replaces other; I don't think it's all the same, is it? How about:

self.is_replaced_by(other)?

So webkit_web_notification_is_replaced_by(self, other);

>> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
>> +            || (coreNotification1 == coreNotification2)
> 
> As I was asking in the previous review, is this superfluous?

s/superfluous/redundant/ =D seems to be, indeed.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:313
> +// Can't use the standard 'kit' name as it clashes with the one defined in WebKitDomNotification
> +WebKitWebNotification* kitNew(WebCore::Notification* coreNotification)

We changed all kit()s that created new instances to kitNew anyway, didn't we? So the name looks good.
------- Comment #47 From 2011-10-27 08:00:57 PST -------
Created an attachment (id=112683) [details]
Patch
------- Comment #48 From 2011-10-27 08:03:05 PST -------
Attachment 112683 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #49 From 2011-11-03 07:53:41 PST -------
(From update of attachment 112683 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=112683&action=review

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:31
> +#if ENABLE(NOTIFICATIONS)

Can you move the #if before the includes as I suggested in the previous two reviews?

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:275
> +char* webkit_web_notification_to_string (WebKitWebNotification* notification)
> +{
> +    return webkit_web_notification_is_html(notification) ?
> +      g_strdup_printf("DESKTOP NOTIFICATION: contents at %s", webkit_web_notification_get_uri(notification)) :
> +      g_strdup_printf("DESKTOP NOTIFICATION:%s icon %s, title %s, text %s",
> +                      webkit_web_notification_get_direction(notification) == GTK_TEXT_DIR_RTL ? "(RTL)" : "",
> +                      webkit_web_notification_get_icon_uri(notification),
> +                      webkit_web_notification_get_title(notification),
> +                      webkit_web_notification_get_body(notification));
> +}

AFAIK this is used only in DRT and quite specific to the tests. Please move this code to DRT, no need to be a function either.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
> +            || (coreNotification1 == coreNotification2)

Can you remove this? As asked in the previous 2 reviews....

>> Source/WebKit/gtk/webkit/webkitwebview.h:85
>> +{
> 
> This { should be at the end of the previous line  [whitespace/braces] [4]

Please fix this
------- Comment #50 From 2011-11-03 08:11:12 PST -------
Hi pnormand,

Thanks for your review.

(In reply to comment #49)
> (From update of attachment 112683 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112683&action=review
> 
> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:31
> > +#if ENABLE(NOTIFICATIONS)
> 
> Can you move the #if before the includes as I suggested in the previous two reviews?
>

Sure.

> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:275
> > +char* webkit_web_notification_to_string (WebKitWebNotification* notification)
> > +{
> > +    return webkit_web_notification_is_html(notification) ?
> > +      g_strdup_printf("DESKTOP NOTIFICATION: contents at %s", webkit_web_notification_get_uri(notification)) :
> > +      g_strdup_printf("DESKTOP NOTIFICATION:%s icon %s, title %s, text %s",
> > +                      webkit_web_notification_get_direction(notification) == GTK_TEXT_DIR_RTL ? "(RTL)" : "",
> > +                      webkit_web_notification_get_icon_uri(notification),
> > +                      webkit_web_notification_get_title(notification),
> > +                      webkit_web_notification_get_body(notification));
> > +}
> 
> AFAIK this is used only in DRT and quite specific to the tests. Please move this code to DRT, no need to be a function either.
> 

Indeed, it is only used in DRT for now but can also be useful in client code (for test/logging purpose). Generally I think it is a good idea to have a default textual representation of an object.
Anyway, I'll move it in the testing code.  

> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
> > +            || (coreNotification1 == coreNotification2)
> 
> Can you remove this? As asked in the previous 2 reviews....
> 

Well, i dont think the comparison are redundant here : the first one check for reference equality of the Gobject wrapper, the second one for the reference ('=' aint overloaded for Notification class). It is here for performance reason, avoiding checking for fields equality if the refs match. A same core Notification might be wrapped by several GObjects even if we try to avoid such a situation.
Thus I am not inclined on removing that check.

> >> Source/WebKit/gtk/webkit/webkitwebview.h:85
> >> +{
> > 
> > This { should be at the end of the previous line  [whitespace/braces] [4]
> 
> Please fix this

False positive, I followed surrounding code format.
------- Comment #51 From 2011-11-09 11:15:31 PST -------
Created an attachment (id=114320) [details]
Patch
------- Comment #52 From 2011-11-09 11:19:55 PST -------
Attachment 114320 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #53 From 2011-11-09 15:57:11 PST -------
(From update of attachment 114320 [details])
Attachment 114320 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10401115
------- Comment #54 From 2011-11-10 02:15:42 PST -------
Created an attachment (id=114456) [details]
Patch
------- Comment #55 From 2011-11-10 02:16:58 PST -------
(In reply to comment #54)
> Created an attachment (id=114456) [details] [details]
> Patch

Updated:
- removed _to_string method
- renamed _replaces by _is_replacement_for
------- Comment #56 From 2011-11-10 02:17:58 PST -------
Attachment 114456 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #57 From 2011-11-10 02:40:44 PST -------
(From update of attachment 114456 [details])
Attachment 114456 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10354326
------- Comment #58 From 2011-11-10 09:14:36 PST -------
Created an attachment (id=114519) [details]
Patch

Updated: install DOM bindings headers
------- Comment #59 From 2011-11-10 09:26:58 PST -------
Attachment 114519 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #60 From 2011-11-10 09:47:10 PST -------
(From update of attachment 114519 [details])
Attachment 114519 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10395373
------- Comment #61 From 2011-11-14 05:58:40 PST -------
Created an attachment (id=114926) [details]
Patch

Update: ifdef-guard webkitwebnotification.h inclusion in webkitwebview.c. Should make EWS happy when building with notifications disabled.
------- Comment #62 From 2011-11-14 06:00:38 PST -------
(In reply to comment #61)
> Created an attachment (id=114926) [details] [details]
> Patch
> 
> Update: ifdef-guard webkitwebnotification.h inclusion in webkitwebview.c. Should make EWS happy when building with notifications disabled.

BTW, is there a way to change EWS compile flags in order to test both situations ($FEATURE disabled and ENABLED)
------- Comment #63 From 2011-11-14 06:01:13 PST -------
Attachment 114926 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/webkit/webkitwebview.h:85:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #64 From 2011-11-14 10:21:15 PST -------
So something we did for WebGL was to make the signals/API there even if WEBGL was disabled.  I think the only reason we'd want to do this differently is if we were unsure if it was going to stick around. Essentially when notifications are disabled the API is still there, but it doesn't do anything. Xan?
------- Comment #65 From 2011-11-21 08:17:08 PST -------
I agree. Having the ABI change depending on configure flags is not very good.
------- Comment #66 From 2012-01-20 17:41:57 PST -------
This bug has gone a bit epic.  Perhaps we can get a gtk reviewer to bring this to a close?  It also may be time to open a new bug and move this work there.
------- Comment #67 From 2012-01-20 17:43:01 PST -------
(From update of attachment 114926 [details])
This looks like a pretty simple patch.  I would expect we should be able to find a gtk reviewer to r+ or r- it. :)
------- Comment #68 From 2012-01-23 07:49:14 PST -------
(From update of attachment 114926 [details])
I'll set this to r- because of the #ifdef'ed API/ABI bits. I assume we have a consensus on the API, though?
------- Comment #69 From 2012-05-15 05:40:42 PST -------
All API so far exists regardless of build configuration. The notification API should do nothing if notifications are disabled, cf. icon database, opengl, Netscape plugins, Java.

The API feels a little complex and doesn't give any examples. I expect the typical use case is libnotify and an app-specific whitelist. It would really help to put that in the docs.
Is is_html compatible with what libnotify expects? If not, how do you convert it?
The picture and document URIs strike me as very inconvenient - how are applications expected to put that into libnotify?
How do notification-request-permission and notification-check-permission work? Both are synchronous with no defined way of saying "I have the answer".
------- Comment #70 From 2013-01-21 04:40:09 PST -------
I'll start looking into continuing with this.
------- Comment #71 From 2013-02-11 08:53:06 PST -------
Created an attachment (id=187590) [details]
Patch
------- Comment #72 From 2013-02-11 09:03:39 PST -------
(In reply to comment #71)
> Created an attachment (id=187590) [details] [details]
> Patch

This patch implements a very basic support for notifications in WK2. It is completely independent from the previous patches.

For this to work, one needs to compile webkit using build-webkit --notifications. Right now, there are no webpages that I know of implementing the latest Web Notifications spec (as defined in http://notifications.spec.whatwg.org ), so I crafted a very simple test ( http://people.gnome.org/~csaavedra/notification.html ).

Some things that are missing or could be improved:

- There is no support yet for notification icons. A notification might have an icon (specified by a URL), in which case the implementation should fetch it and pass a GdkPixbuf to libnotify. I am not sure how to implement this, as I think it would be wise to cache this somehow.

- The libnotify specific code is all in WebKit2/API/gtk/WebKitNotificationProvider.cpp. We could abstract this and add a LibnotifyNotificationProvider that would go into Platform instead, and make WebKitNotificationProvider subclassable, so that applications could implement their own providers, independent from libnotify. In general, I think the whole API is up for discussion, as you can see the patch this is still pretty bare.

- I am abusing g_object_set_data() in order to store the NotificationID in the NotifyNotification. I suppose we might want to add a class that would wrap NotifyNotification or something like that, not to depend on those implementation details.

That's all I can think of for now.
------- Comment #73 From 2013-02-12 02:31:17 PST -------
Created an attachment (id=187815) [details]
Patch
------- Comment #74 From 2013-02-12 02:41:24 PST -------
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
------- Comment #75 From 2013-02-12 02:41:42 PST -------
Attachment 187815 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/GNUmakefile.am', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPermissionRequest.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPermissionRequest.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPermissionRequestPrivate.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h', u'Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp', u'Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp', u'Source/WebKit2/UIProcess/API/gtk/docs/webkit2gtk-sections.txt', u'Source/WebKit2/UIProcess/API/gtk/webkit2.h', u'Source/autotools/FindDependencies.m4', u'Source/autotools/PrintBuildConfiguration.m4', u'Source/autotools/ReadCommandLineArguments.m4', u'Tools/ChangeLog', u'Tools/MiniBrowser/gtk/BrowserWindow.c', u'Tools/gtk/generate-gtkdoc']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h"
Tools/MiniBrowser/gtk/BrowserWindow.c:368:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/webkit2.h"
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPermissionRequest.h"
Total errors found: 1 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #76 From 2013-02-12 02:49:08 PST -------
(From update of attachment 187815 [details])
Attachment 187815 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/16454151
------- Comment #77 From 2013-02-12 07:41:21 PST -------
(From update of attachment 187815 [details])
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.
------- Comment #78 From 2013-02-14 16:04:29 PST -------
(From update of attachment 187815 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=187815&action=review

>> Source/autotools/ReadCommandLineArguments.m4:132
>> +
> 
> 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.

I wouldn't mind making it a requirement tbh (geoclue, I mean)
------- Comment #79 From 2013-05-07 11:20:46 PST -------
Hi, I tried your patch. It works fine. But it crashes webkit1. May be you need to write a stub NotificationClient implementation, to avoid at least crashing.
------- Comment #80 From 2013-05-07 16:42:23 PST -------
(In reply to comment #79)
> Hi, I tried your patch. It works fine. But it crashes webkit1. May be you need to write a stub NotificationClient implementation, to avoid at least crashing.

IIRC it crashes because of bug 108482.
------- Comment #81 From 2013-09-17 12:19:55 PST -------
Hey Claudio, do you have plans to finish this?
------- Comment #82 From 2013-09-17 23:42:19 PST -------
Not at the moment.
------- Comment #83 From 2013-10-22 00:58:00 PST -------
There is a new GNotification API in GLib. We should probably use it instead of libnotify. See https://bugzilla.gnome.org/show_bug.cgi?id=688492