Bug 73253 – Create skeleton framework for notifications support in WK2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jon Lee]

This represents part 1 of bringing WK2 support of notifications. Basic showing and canceling of notifications, with no tracking, as well as exposing relevant classes as WK API.

[Jon Lee]

Created attachment 116838 [details]
Patch

[Sam Weinig]

Comment on attachment 116838 [details]
Patch

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.

[Jon Lee]

Created attachment 116843 [details]
Patch

[Jon Lee]

Created attachment 116848 [details]
Fix merge issues for EWS

[Sam Weinig]

Comment on attachment 116848 [details]
Fix merge issues for EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=116848&action=review

> Source/WebKit2/Shared/WebCoreArgumentCoders.h:47
> +    class Notification;

Seems unused.

[Jon Lee]

Committed r101307: <http://trac.webkit.org/changeset/101307>

[Jon Lee]

*** Bug 72743 has been marked as a duplicate of this bug. ***

[Zhenyao Mo]

fast/notifications/notification-after-close.html
fast/notifications/notifications-check-permission.html
fast/notifications/notifications-click-event.html
fast/notifications/notifications-display-close-events.html
fast/notifications/notifications-double-show.html
fast/notifications/notifications-no-icon.html
fast/notifications/notifications-replace.html
fast/notifications/notifications-request-permission.html
fast/notifications/notifications-rtl.html
fast/notifications/notifications-with-permission.html
fast/notifications/notifications-without-permission.html

11 tests crashing in chromium linux (not sure about mac and win yet)

I am going to clobber a linux bot and do a clean build/run.  if it's still crashing, I'll have to revert this patch...