[webkit-reviews] review denied: [Bug 61140] [GTK] Add HTML5 Notifications support : [Attachment 108810] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Sep 27 09:41:50 PDT 2011
Philippe Normand <pnormand at igalia.com> has denied Alexandre Mazari
<scaroo at gmail.com>'s request for review:
Bug 61140: [GTK] Add HTML5 Notifications support
https://bugs.webkit.org/show_bug.cgi?id=61140
Attachment 108810: Patch
https://bugs.webkit.org/attachment.cgi?id=108810&action=review
------- Additional Comments from Philippe Normand <pnormand at igalia.com>
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
More information about the webkit-reviews
mailing list