[webkit-reviews] review denied: [Bug 73253] Create skeleton framework for notifications support in WK2 : [Attachment 116838] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Jon Lee <jonlee at apple.com>'s request for
review:
Bug 73253: Create skeleton framework for notifications support in WK2
https://bugs.webkit.org/show_bug.cgi?id=73253

Attachment 116838: Patch
https://bugs.webkit.org/attachment.cgi?id=116838&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116838&action=review


> Source/WebCore/notifications/NotificationController.cpp:38
> +: m_page(page)
> +, m_client(client)
> +{

Indent please.

> Source/WebKit2/Shared/UserMessageCoders.h:183
> +	   case APIObject::TypeNotification: {
> +	       WebNotification* notification =
static_cast<WebNotification*>(m_root);
> +	       encoder->encode(notification->title());
> +	       encoder->encode(notification->body());
> +	       return true;
> +	   }

I don't think you need to make WebNotifications user encodable.

> Source/WebKit2/Shared/WebNotification.h:43
> +class WebNotification : public APIObject {
> +public:

Since this will probably only bne used from the UIProcess, you can put this in
that directory, instead of shared.

> Source/WebKit2/UIProcess/API/C/WKNotification.cpp:31
> +#include "WebNotificationManagerProxy.h"

This doesn't seem to be used.

> Source/WebKit2/UIProcess/API/C/WKNotification.h:38
> +WK_EXPORT WKStringRef WKNotificationCopyTitle(WKNotificationRef);
> +WK_EXPORT WKStringRef WKNotificationCopyBody(WKNotificationRef);

We usually put parameter names in API headers.

> Source/WebKit2/UIProcess/API/C/WKNotificationManager.h:37
> +WK_EXPORT void WKNotificationManagerSetProvider(WKNotificationManagerRef,
const WKNotificationProvider*);

We usually put parameter names in API headers.

> Source/WebKit2/UIProcess/API/C/WKNotificationProvider.h:36
> +typedef void (*WKNotificationProviderShowCallback)(WKNotificationRef, const
void* clientInfo);
> +typedef void (*WKNotificationProviderCancelCallback)(WKNotificationRef,
const void* clientInfo);

We usually put parameter names in API headers.

> Source/WebKit2/UIProcess/API/C/WKNotificationProvider.h:42
> +    int version;
> +    const void* clientInfo;
> +    WKNotificationProviderShowCallback show;
> +    WKNotificationProviderCancelCallback cancel;

We usually align the struct member names.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:36
> +#if ENABLE(NOTIFICATIONS)
> +#include "WebNotificationManagerProxyMessages.h"
> +#endif
> +#include "WebPage.h"
> +#include "WebProcess.h"
> +#if ENABLE(NOTIFICATIONS)
> +#include <WebCore/Notification.h>
> +#endif

Please group the ENABLE(NOTIFICATIONS) together.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:44
> +{ }

Two lines.