[Webkit-unassigned] [Bug 61140] [GTK] Add HTML5 Notifications support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 23 11:34:30 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=61140
Martin Robinson <mrobinson at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #94111|review? |review-
Flag| |
--- Comment #4 from Martin Robinson <mrobinson at webkit.org> 2011-05-23 11:34:30 PST ---
(From update of attachment 94111)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list