[webkit-reviews] review denied: [Bug 131483] [EFL][WK2] Implement web notification. : [Attachment 229033] Patch

[bugzilla-daemon at webkit.org]

Gyuyoung Kim <gyuyoung.kim at samsung.com> has denied SangYong Park
<sy302.park at samsung.com>'s request for review:
Bug 131483: [EFL][WK2] Implement web notification.
https://bugs.webkit.org/show_bug.cgi?id=131483

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

------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
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_Req
uest *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.