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

[bugzilla-daemon at webkit.org]

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


Philippe Normand <pnormand at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #108810|review?                     |review-
               Flag|                            |




--- Comment #45 from Philippe Normand <pnormand at igalia.com>  2011-09-27 09:41:51 PST ---
(From update of attachment 108810)
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

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