Bug 61140

Created attachment 94111 [details] Patch

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 on attachment 94111 [details] Patch Attachment 94111 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8719196

Comment on attachment 94111 [details] Patch 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.

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]) > 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.

(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.

Created attachment 97599 [details] Patch

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.

(In reply to comment #7) > Created an attachment (id=97599) [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 on attachment 97599 [details] Patch Attachment 97599 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8881560

Created attachment 97796 [details] Patch

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 on attachment 97796 [details] Patch Attachment 97796 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8907596

Created attachment 97847 [details] Patch

Created attachment 97957 [details] Patch

(In reply to comment #15) > Created an attachment (id=97957) [details] > Patch - Added documentation for WebKitWebNotificationPresenter interface - remove unused code - clean copyright headers

Comment on attachment 97957 [details] Patch Attachment 97957 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8909952

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.

(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 on attachment 97957 [details] Patch 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/<//

(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 !

(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!

(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.

Created attachment 99070 [details] Patch

Comment on attachment 99070 [details] Patch Attachment 99070 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8965021

Created attachment 99106 [details] Patch

(In reply to comment #26) > Created an attachment (id=99106) [details] > Patch Going back to the signals implementations as requested.

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 on attachment 99106 [details] Patch Attachment 99106 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8896013

Created attachment 99450 [details] Patch Added ChangeLog entry

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.

Created attachment 99453 [details] Patch Fix style error in Source/WebKit/gtk/ChangeLog

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 on attachment 99453 [details] Patch Attachment 99453 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/8970424

(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 on attachment 99453 [details] Patch 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?

Created attachment 104352 [details] Patch

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.

Created attachment 104354 [details] Patch

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.

(In reply to comment #39) > Created an attachment (id=104354) [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 on attachment 104354 [details] Patch 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" ?

Created attachment 108810 [details] Patch

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 on attachment 108810 [details] Patch 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 on attachment 108810 [details] Patch 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.

Created attachment 112683 [details] Patch

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 on attachment 112683 [details] Patch 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

Hi pnormand, Thanks for your review. (In reply to comment #49) > (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? > 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.

Created attachment 114320 [details] Patch

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 on attachment 114320 [details] Patch Attachment 114320 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10401115

Created attachment 114456 [details] Patch

(In reply to comment #54) > Created an attachment (id=114456) [details] > Patch Updated: - removed _to_string method - renamed _replaces by _is_replacement_for

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 on attachment 114456 [details] Patch Attachment 114456 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10354326

Created attachment 114519 [details] Patch Updated: install DOM bindings headers

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 on attachment 114519 [details] Patch Attachment 114519 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10395373

Created attachment 114926 [details] Patch Update: ifdef-guard webkitwebnotification.h inclusion in webkitwebview.c. Should make EWS happy when building with notifications disabled.

(In reply to comment #61) > 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. BTW, is there a way to change EWS compile flags in order to test both situations ($FEATURE disabled and ENABLED)

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.

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?

I agree. Having the ABI change depending on configure flags is not very good.

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 on attachment 114926 [details] Patch 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 on attachment 114926 [details] Patch I'll set this to r- because of the #ifdef'ed API/ABI bits. I assume we have a consensus on the API, though?

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".

I'll start looking into continuing with this.

Created attachment 187590 [details] Patch

(In reply to comment #71) > Created an attachment (id=187590) [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.

Created attachment 187815 [details] Patch

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

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 on attachment 187815 [details] Patch Attachment 187815 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/16454151

Comment on attachment 187815 [details] Patch 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 on attachment 187815 [details] Patch 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)

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.

(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.

Hey Claudio, do you have plans to finish this?

Not at the moment.

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

Comment on attachment 187815 [details] Patch Some style issues and needs to be updated to use glib's new APIs, I'll try to do that during the hackfest, I have a growing interest in notifications =)

Created attachment 242894 [details] Patch Uploading the patch so we can get the API reviewed, I will still port it to libnotify

Comment on attachment 242894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242894&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:62 > + String notificationIDString = String::format("webkit:%" PRIu64, notification->priv->id); > + g_application_withdraw_notification(application, notificationIDString.utf8().data()); We could use g_strdup_printf here to avoid converting from/to utf8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:97 > + */ Since: 2.8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:110 > + */ Since: 2.8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:116 > + 0, nullptr > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:123 > + */ Since: 2.8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:129 > + 0, nullptr > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:148 > + G_STRUCT_OFFSET(WebKitNotificationClass, cancel), I don't think we should have a vmethod in the class struct, since the user can't create new instances, and inheritance is not possible. For the default implementation we could check the return value after emitting the signal. > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:154 > +WebKitNotification* webkitNotificationCreate(WKNotificationRef notificationRef) Do not use the C API, use const WebNotification& instead. > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:156 > + WebKitNotification* notification = WEBKIT_NOTIFICATION(g_object_new(WEBKIT_TYPE_NOTIFICATION, NULL)); nullptr > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:176 > + */ Since: 2.8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:191 > + */ Since: 2.8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:194 > + g_return_val_if_fail(WEBKIT_IS_NOTIFICATION(notification), 0); nullptr > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:206 > + */ Since: 2.8 > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:209 > + g_return_val_if_fail(WEBKIT_IS_NOTIFICATION(notification), 0); nullptr > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.h:53 > + gboolean (* cancel) (WebKitNotification *notification); I would avoid this if possible. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPermissionRequest.cpp:100 > + WebKitNotificationPermissionRequest* notificationPermissionRequest = WEBKIT_NOTIFICATION_PERMISSION_REQUEST(g_object_new(WEBKIT_TYPE_NOTIFICATION_PERMISSION_REQUEST, NULL)); nullptr > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:46 > + toNotificationProvider(clientInfo)->show(page, notification); pass toImpl(page) and toImpl(notification) here > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:51 > + toNotificationProvider(clientInfo)->cancel(notification); Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:65 > + , m_notifications() This is not needed. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:84 > +void WebKitNotificationProvider::show(WKPageRef page, WKNotificationRef notificationRef) this should receive a WebPageProxy* and const WebNotification& and you don't need all the toImpl() here > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:89 > + notification = webkitNotificationCreate(notificationRef); Who is taking the ownership of this notification? Shouldn't we use GRefPtr in the m_notifications hashmap? > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:98 > +void WebKitNotificationProvider::cancel(WKNotificationRef notificationRef) Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:106 > + WebKitNotification* notification = m_notifications.get(toImpl(notificationRef)->notificationID()); > + if (!notification) > + return; > + > + webkitNotificationEmitCancel(notification); if (WebKitNotification* notification = m_notifications.get(webNotification.notificationID())) webkitNotificationEmitCancel(notification); > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.h:37 > + void show(WKPageRef, WKNotificationRef); > + void cancel(WKNotificationRef); Do these need to be public? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:570 > +static gboolean webkitWebViewShowNotification(WebKitWebView*, WebKitNotification* web_notification) It's a bit weird that the default implementation for show is in the view, but the cancel is in the notification object. Maybe we could have two signals in the view instead. SHOW/HIDE or SHOW/CANCEL? I think it would be easier for application overriding the behaviour inheriting from WebKitWebView. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:578 > + GRefPtr<GNotification> notification = adoptGRef(g_notification_new(web_notification->priv->title.data())); > + g_notification_set_body(notification.get(), web_notification->priv->body.data()); I think we could use the public API and move the private struct to the cpp file > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:582 > + String notificationIDString = String::format("webkit:%" PRIu64, web_notification->priv->id); > + g_application_send_notification(application, notificationIDString.utf8().data(), notification.get()); g_strdup_printf > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:633 > + webkit_web_view_run_javascript(m_webView, "Notification.requestPermission()", 0, 0, 0); nullptr > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:639 > + webkit_web_view_run_javascript(m_webView, "n = new Notification('This is a notification', { body: 'This is the body.'});", 0, 0, 0); nullptr > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:645 > + webkit_web_view_run_javascript(m_webView, "n.close()", 0, 0, 0); nullptr > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:655 > + // Notifications don't work with local or special schemes. > + test->loadHtml("<body></body>", "http://webkitgtk.org/"); We should use soup server instead then

Created attachment 242910 [details] Patch

Created attachment 242926 [details] Patch

Created attachment 242931 [details] Patch

Created attachment 242932 [details] Patch

Created attachment 242935 [details] Patch

Patch for Epiphany to take advantage of this: https://bugzilla.gnome.org/show_bug.cgi?id=741295

Comment on attachment 242935 [details] Patch Attachment 242935 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6162910405459968 New failing tests: http/tests/notifications/legacy/events.html http/tests/notifications/events.html

Created attachment 242938 [details] Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

Created attachment 242940 [details] Patch

Comment on attachment 242935 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242935&action=review I guess we should add libnotify to the jhbuild moduleset. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPrivate.h:40 > +#if USE(LIBNOTIFY) > +#include <libnotify/notify.h> > +#endif > + > +struct _WebKitNotificationPrivate { > + CString title; > + CString body; > + guint64 id; > + > +#if USE(LIBNOTIFY) > + NotifyNotification* notifyNotification; > +#endif > +}; I prefer to move this to the cpp file if possible > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:99 > + m_notificationManager->providerDidShowNotification(webNotification.notificationID()); Maybe we should do tis only if the notification was actually shown? The user might connect to the show-notification signal and just return TRUE. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1622 > + * The default handler will close the notification using libnotify, if built with > + * support for it. That makes me thing whether it should be called close instead of cancel. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2039 > +#if USE(LIBNOTIFY) > + if (handled) > + return; > + > + if (!notify_is_initted()) > + notify_init(g_get_prgname()); > + > + NotifyNotification* notification = webNotification->priv->notifyNotification; > + if (!notification) > + notification = notify_notification_new(webNotification->priv->title.data(), webNotification->priv->body.data(), nullptr); > + else > + notify_notification_update(notification, webNotification->priv->title.data(), webNotification->priv->body.data(), nullptr); > + > + notify_notification_show(notification, nullptr); > +#endif Since the webview has he vmethods, this could be moved to the vmethod default impl, my suggestion before was only for the case of WebKitNotification, because it didn't make sense to have a vmethod there. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2056 > +#if USE(LIBNOTIFY) > + if (handled) > + return; > + > + if (!webNotification->priv->notifyNotification) > + return; > + > + notify_notification_close(webNotification->priv->notifyNotification, nullptr); > + return; > +#endif Ditto. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:241 > + And add the vmethods here, sorry for confusing you in my previous review. > Source/WebKit2/UIProcess/Notifications/WebNotificationProvider.h:36 > - typedef std::tuple<WKNotificationProviderV0> Versions; > + typedef std::tuple<WKNotificationProviderV1> Versions; I don't think this is correct, you should add the new version typedef std::tuple<WKNotificationProviderV0, WKNotificationProviderV1> Versions;

Comment on attachment 242940 [details] Patch Attachment 242940 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6571284485898240 New failing tests: http/tests/notifications/legacy/events.html http/tests/notifications/events.html

Created attachment 242954 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

(In reply to comment #96) > Comment on attachment 242935 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242935&action=review > > I guess we should add libnotify to the jhbuild moduleset. Probably just install-dependencies, unless there is a version requirement for tests. :)

(In reply to comment #99) > (In reply to comment #96) > > Comment on attachment 242935 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=242935&action=review > > > > I guess we should add libnotify to the jhbuild moduleset. > > Probably just install-dependencies, unless there is a version requirement > for tests. :) Agree, much better.

We don't need to add libnotify to the bots, actually, it is not required to enable and test the feature, just for the default implementation of the signal to work, which we do not test anyway.

Created attachment 242997 [details] Patch This should fix the mac tests

Created attachment 242998 [details] Patch

+ * Copyright (C) 2014 Collabota Ltd. :)

Created attachment 243010 [details] Patch Another version which does not need the WK2 API changes! =)

Comment on attachment 243010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243010&action=review > Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:121 > +WebKitNotification* webkitNotificationCreate(const WebKit::WebNotification& webNotification) I think the WebView/WebPage should be passed here to set the web view member. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPrivate.h:42 > +#if USE(LIBNOTIFY) > +#include <libnotify/notify.h> > +#endif > + > +struct _WebKitNotificationPrivate { > + CString title; > + CString body; > + guint64 id; > + > + WebKitWebView* webView; > + > +#if USE(LIBNOTIFY) > + NotifyNotification* notifyNotification; > +#endif > +}; I think you missed one of my reviews, or you are ignoring me :-D > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:94 > + notification = adoptGRef(webkitNotificationCreate(webNotification)); pass the page here > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:100 > + m_notificationManager->providerDidShowNotification(webNotification.notificationID()); Maybe we should do this only if the notification was actually shown? The user might connect to the show-notification signal and just return TRUE. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:111 > +void WebKitNotificationProvider::clearNotifications(const API::Array* notificationIDs) So this is to clear all the notifications or only the notifications of a given page, for example when the page is closed/destroyed? > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:113 > + for (size_t i = 0; i < notificationIDs->size(); i++) { You use a modern loop, something like for (const auto& item : notificationIDs->elements()) > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:116 > + webkitWebViewEmitCancelNotification(WEBKIT_WEB_VIEW(notification->priv->webView), notification.get()); This cast is not needed, notification->priv->webView is already a WebKitWebView. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1621 > + * The default handler will close the notification using libnotify, if built with > + * support for it. That makes me thing whether it should be called close instead of cancel. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2021 > + webNotification->priv->webView = webView; This looks weird to me, the notification should be created with a web view, I think. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2037 > + NotifyNotification* notification = webNotification->priv->notifyNotification; > + if (!notification) > + notification = notify_notification_new(webNotification->priv->title.data(), webNotification->priv->body.data(), nullptr); > + else > + notify_notification_update(notification, webNotification->priv->title.data(), webNotification->priv->body.data(), nullptr); Since all the default libnotifiy impl is here, what do you think about keeping a hash map in the view to map WebKitNotification to NotifyNotification, that is added here and removed in cancel, and we leave WebKitNotification independent from the implementation? > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:2057 > +#if USE(LIBNOTIFY) > + if (handled) > + return; > + > + if (!webNotification->priv->notifyNotification) > + return; > + > + notify_notification_close(webNotification->priv->notifyNotification, nullptr); > + return; > +#endif Since the webview has the vmethods, this could be moved to the vmethod default impl, my suggestion before was only for the case of WebKitNotification, because it didn't make sense to have a vmethod there. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:606 > + webkit_permission_request_allow(request); what happens if do deny? I guess show is neer emitted and the notification is not even created. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:641 > + webkit_web_view_run_javascript(m_webView, "Notification.requestPermission();", nullptr, nullptr, nullptr); we should set the event to none here > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:649 > + void requestNotificationAndWaitUntilShown() > + { > + webkit_web_view_run_javascript(m_webView, "n = new Notification('This is a notification', { body: 'This is the body.'});", nullptr, nullptr, nullptr); > + g_main_loop_run(m_mainLoop); > + } We could make this more generic, and pass the title and body as parameters so that the caller can check that what was passed is what is stored in the notification object. WE should also reset the event here as well. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:653 > + webkit_web_view_run_javascript(m_webView, "n.close()", nullptr, nullptr, nullptr); And here. > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:674 > + test->requestNotificationAndWaitUntilShown(); > + > + g_assert(test->m_event == NotificationWebViewTest::Shown); > + The WebKitNotification API is actually not tested, we should check here that the title, body, etc. match to what we expect.

Comment on attachment 243010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243010&action=review >> Source/WebKit2/UIProcess/API/gtk/WebKitNotification.cpp:121 >> +WebKitNotification* webkitNotificationCreate(const WebKit::WebNotification& webNotification) > > I think the WebView/WebPage should be passed here to set the web view member. Indeed, intended to do so and forgot while banging my head against a phantom issue heh. >> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationPrivate.h:42 >> +}; > > I think you missed one of my reviews, or you are ignoring me :-D Err, very sorry about this, I intended to ask you about this and forgot! >> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:100 >> + m_notificationManager->providerDidShowNotification(webNotification.notificationID()); > > Maybe we should do this only if the notification was actually shown? The user might connect to the show-notification signal and just return TRUE. Agreed. Well, we cannot know if a notification was actually shown, but we can do this only if something handled the signal anyway. >> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:111 >> +void WebKitNotificationProvider::clearNotifications(const API::Array* notificationIDs) > > So this is to clear all the notifications or only the notifications of a given page, for example when the page is closed/destroyed? As we discussed, only the notifications that are passed in, so those from the page when there is a navigation - this is what gets called when the second loadURI happens in the test. >> Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:113 >> + for (size_t i = 0; i < notificationIDs->size(); i++) { > > You use a modern loop, something like for (const auto& item : notificationIDs->elements()) Aha, thanks! >> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:606 >> + webkit_permission_request_allow(request); > > what happens if do deny? I guess show is neer emitted and the notification is not even created. exactly

Created attachment 243014 [details] Patch Sorry about missing your previous review, this should cover all the comments =)

Comment on attachment 243014 [details] Patch Attachment 243014 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4900243707527168 New failing tests: fast/canvas/webgl/tex-image-and-sub-image-2d-with-canvas.html

Created attachment 243030 [details] Archive of layout-test-results from ews100 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 243014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243014&action=review Super!, just check the few nits before landing, please. > Source/WebKit2/UIProcess/API/gtk/WebKitNotificationProvider.cpp:117 > + if (GRefPtr<WebKitNotification> notification = m_notifications.get(notificationID)) > + webkitWebViewEmitCloseNotification(webkitNotificationGetWebView(notification.get()), notification.get()); > + > + m_notifications.remove(notificationID); This ode is mostly a duplicate of cancel(), we could move it to a helper private function that receives the notification id, for example. > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:157 > +typedef HashMap<uint64_t, GRefPtr<NotifyNotification> > NotifyNotificationsMap; We no longer need the space in > > with C++ 11 > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:635 > + g_signal_connect(m_webView, "permission-request", G_CALLBACK(permissionRequestCallback), this); > + g_signal_connect(m_webView, "show-notification", G_CALLBACK(showNotificationCallback), this); > + g_signal_connect(m_webView, "close-notification", G_CALLBACK(closeNotificationCallback), this); Use g_signal_disconnect_matched in the destructor to disconnect all te.hse > Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebKitWebView.cpp:678 > + const char* title = "This is a notification"; > + const char* body = "This is the body."; These could be static.

Committed r177073: <http://trac.webkit.org/changeset/177073>