Bug 131483 – [EFL][WK2] Implement web notification.

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[SangYong Park]

Implement webkit2 EFL APIs of web notification.

[SangYong Park]

Created attachment 229033 [details]
Patch

[Jinwoo Song]

Comment on attachment 229033 [details]
Patch

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

API unit test should be added.(maybe in other bug.)

> Source/WebKit2/UIProcess/API/efl/ewk_notification.cpp:55
> +    m_ewkView = 0;

s/0/nullptr

> Source/WebKit2/UIProcess/API/efl/ewk_notification_permission_request_private.h:51
> +    explicit EwkNotificationPermissionRequest(WKNotificationPermissionRequestRef, WKSecurityOriginRef);

We do not need *explicit* keyword here.

> Source/WebKit2/UIProcess/API/efl/ewk_notification_private.h:64
> +    explicit EwkNotification(WKNotificationRef, EwkView*);

We do not need *explicit* keyword here.

[Ryuan Choi]

Comment on attachment 229033 [details]
Patch

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

Good. I commented bit.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:83
> +#if ENABLE(NOTIFICATIONS)
> +    , m_notificationProvider(std::make_unique<NotificationProvider>(context))
> +#endif

How about creating it as lazy as possible?

And, should we add it in not ewk_view but ewk_context ?

> Source/WebKit2/UIProcess/API/efl/ewk_notification.cpp:51
> +{ }

We use different line for }

> Source/WebKit2/UIProcess/efl/NotificationProvider.h:39
> +class NotificationProvider {

NotificationProviderEfl?

[Gyuyoung Kim]

Comment on attachment 229033 [details]
Patch

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

I think this patch needs to be reviewed for a long time because patch is too huge. :(

> Source/WebKit/efl/ChangeLog:3
> +        [EFL][WK2] Implement web notification.

It would be good if you add [WK1] prefix as well.

> Source/WebKit2/UIProcess/API/efl/ewk_notification.h:58
> + *         save yourself from cpu cycles and use eina_stringshare_ref()

I don't understand why this return type can be guaranteed by eina_stringshare. is the return char* protected by eina_stringshare_add() ?

> Source/WebKit2/UIProcess/API/efl/ewk_notification.h:61
> +EAPI const char *ewk_notification_title_get(const Ewk_Notification *notification);

Any unit test for this new APIs ?

> Source/WebKit2/UIProcess/API/efl/ewk_notification.h:128
> + * @return the security origin object or @c NULL if there is not security origin.

Do not use period at @return field.

> Source/WebKit2/UIProcess/API/efl/ewk_notification_permission_request.h:58
> +EAPI Ewk_Security_Origin *ewk_notification_permission_request_security_origin_get(const Ewk_Notification_Permission_Request *permissionRequest);

Any unit test for this new APIs ?

permissionRequest -> permission_request

> Source/WebKit2/UIProcess/API/efl/ewk_notification_permission_request.h:66
> +EAPI void ewk_notification_permission_request_allowed_set(Ewk_Notification_Permission_Request *permissionRequest, Eina_Bool allow);

ditto.

> Source/WebKit2/UIProcess/efl/NotificationProvider.h:26
> +#ifndef NotificationProvider_h

EFL port has used *Efl* post fix. Please use NotificaitonProviderEfl_h for file name as well.

[SangYong Park]

Created attachment 229867 [details]
Patch

[SangYong Park]

I will implement API and add unit test in additional patch.

[Gyuyoung Kim]

Comment on attachment 229867 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:-451
> -NotificationClient* ChromeClientEfl::notificationPresenter() const

Please remove this deprecated function in new bug.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:-117
> -#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)

ditto.

> Tools/Scripts/webkitperl/FeatureList.pm:351
> +      define => "ENABLE_NOTIFICATIONS", default => isEfl(), value => \$notificationsSupport },

I think we can enable this feature by default when implementation is finished.

[SangYong Park]

Created attachment 231491 [details]
Patch

[SangYong Park]

Comment on attachment 229867 [details]
Patch

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

>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:-451
>> -NotificationClient* ChromeClientEfl::notificationPresenter() const
> 
> Please remove this deprecated function in new bug.

done

>> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:-117
>> -#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> 
> ditto.

done

>> Tools/Scripts/webkitperl/FeatureList.pm:351
>> +      define => "ENABLE_NOTIFICATIONS", default => isEfl(), value => \$notificationsSupport },
> 
> I think we can enable this feature by default when implementation is finished.

done

[Gyuyoung Kim]

Comment on attachment 231491 [details]
Patch

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

> Source/WebKit2/PlatformEfl.cmake:380
> +    "${CMAKE_CURRENT_SOURCE_DIR}/UIProcess/API/efl/ewk_notification.h"

It would be good if you open this new API files when you finish the implementation.

> Source/WebKit2/UIProcess/API/efl/EWebKit2.h:48
> +#include "ewk_notification.h"

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:78
> +    , m_notificationProvider(std::make_unique<NotificationProviderEfl>(context))

Can I ask why you need to have ownership of m_notificationProvider ? If this is only for passing an pointer to WebCore side, I think we don't need to have this member variable and ownership.

[Gyuyoung Kim]

Comment on attachment 231491 [details]
Patch

Cleared review? from attachment 231491 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again