Bug 131483 - [EFL][WK2] Implement web notification.
Summary: [EFL][WK2] Implement web notification.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-04-10 01:08 PDT by SangYong Park
Modified: 2014-06-23 21:32 PDT (History)
8 users (show)

See Also:


Attachments
Patch (62.22 KB, patch)
2014-04-10 01:53 PDT, SangYong Park
no flags Details | Formatted Diff | Diff
Patch (42.77 KB, patch)
2014-04-21 23:34 PDT, SangYong Park
no flags Details | Formatted Diff | Diff
Patch (39.04 KB, patch)
2014-05-14 22:27 PDT, SangYong Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description SangYong Park 2014-04-10 01:08:58 PDT
Implement webkit2 EFL APIs of web notification.
Comment 1 SangYong Park 2014-04-10 01:53:12 PDT
Created attachment 229033 [details]
Patch
Comment 2 Jinwoo Song 2014-04-10 21:57:16 PDT
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.
Comment 3 Ryuan Choi 2014-04-10 22:42:02 PDT
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?
Comment 4 Gyuyoung Kim 2014-04-11 00:36:59 PDT
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.
Comment 5 SangYong Park 2014-04-21 23:34:56 PDT
Created attachment 229867 [details]
Patch
Comment 6 SangYong Park 2014-04-22 01:32:28 PDT
I will implement API and add unit test in additional patch.
Comment 7 Gyuyoung Kim 2014-05-14 21:14:43 PDT
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.
Comment 8 SangYong Park 2014-05-14 22:27:40 PDT
Created attachment 231491 [details]
Patch
Comment 9 SangYong Park 2014-05-14 22:29:21 PDT
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
Comment 10 Gyuyoung Kim 2014-05-14 22:37:38 PDT
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.
Comment 11 Gyuyoung Kim 2014-06-23 18:33:46 PDT
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