[webkit-reviews] review denied: [Bug 73544] [EFL] Support for Web Notification. : [Attachment 148060] 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 148060: Patch.
https://bugs.webkit.org/attachment.cgi?id=148060&action=review

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


Could you let me know what is difference between NOTIFICATION feature and
LEGACY_NOTIFICATIONS? It is a little difficult to understand the difference in
this patch.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-24
> -#include "NotImplemented.h"

When NOTIFICATION is disabled, there is build break as below,

86%] Building CXX object
Source/WebKit/CMakeFiles/ewebkit.dir/efl/WebCoreSupport/PageClientEfl.cpp.o
In file included from
/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPrese
nterClientEfl.cpp:21:0:
/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPrese
nterClientEfl.h:24:26: fatal error: Notification.h: No such file or directory

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> +    // Called from ~Notification(), Remove the entry from the notifications
list.

s/Remove/remove/g

> Source/WebKit/efl/ewk/ewk_notification.cpp:131
> +    return strdup(ewkNotification->notification->replaceId().utf8().data());


When LEGACY_NOTIFICATION is disabled, there is a build break. We have to
support perfect build regardless of feature dependency.

/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/ewk/ewk_notification.cpp: In
function ‘const char* ewk_notification_replace_id_get(const
Ewk_Notification*)’:
/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/ewk/ewk_notification.cpp:131:50:
error: ‘class WebCore::Notification’ has no member named ‘replaceId’
[ 99%] Built target ewebkit2