[webkit-reviews] review denied: [Bug 73544] [EFL] Support for Web Notification. : [Attachment 146824] Patch.

[bugzilla-daemon at webkit.org]

Gyuyoung Kim <gyuyoung.kim at samsung.com> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 73544: [EFL] Support for Web Notification.
https://bugs.webkit.org/show_bug.cgi?id=73544

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

------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=146824&action=review


> Source/WebKit/PlatformEfl.cmake:79
>  IF (ENABLE_NOTIFICATIONS)

I think you can add this to source list unconditionally because
ewk_notification.cpp is already protected by #ifdef.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:48
> +	       if (iter->first->tag() == notification->tag() &&
iter->first->url().protocol() == notification->url().protocol() &&
iter->first->url().host() == notification->url().host()) {

It looks it is better to use multiple lines for this conditions.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:91
> +    evas_object_smart_callback_call(m_view,
"notification,permission,request",
static_cast<void*>(const_cast<char*>(domain.utf8().data())));

I'm not sure whether multicasting is safe like this. EFL port has used (void*)
C-type casting instead of C++ casting for this.

I think you can add internal functions to send signal to ewk_notificaiton.cpp

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:113
> +	   evas_object_smart_callback_call(m_view,
"notification,permission,cancel",
static_cast<void*>(const_cast<char*>(domain.utf8().data())));

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:83
> +EAPI const char *ewk_notification_title_get(Ewk_Notification *en);

As mentioned in comment #53, _get() needs to use *const*.

> Source/WebKit/efl/ewk/ewk_notification.h:92
> +EAPI const char *ewk_notification_icon_url_get(Ewk_Notification *en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:101
> +EAPI const char *ewk_notification_body_get(Ewk_Notification *en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:110
> +EAPI const char *ewk_notification_replace_id_get(Ewk_Notification *en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:119
> +EAPI const char *ewk_notification_dir_get(Ewk_Notification *en);

ditto.

> Tools/ChangeLog:9
> +

Missing changed file in ChangeLog.