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

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


Please read WebKit EFL coding style guideline.

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

s/Remove/remove/g

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:45
> +    virtual void requestPermission(ScriptExecutionContext*,
PassRefPtr<NotificationPermissionCallback>) { }

I couldn't find implementation for this virtual function. Don't you need to
implement this ?

> Source/WebKit/efl/ewk/ewk_notification.cpp:32
> +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)

Is it better to add comment for these internal functions ?

> Source/WebKit/efl/ewk/ewk_notification.h:38
> +    char* domain;

Move '*' to variable side.

> Source/WebKit/efl/ewk/ewk_notification.h:39
> +    bool isAllowed;

s/isAllowed/allow/g. In addition, use Eina_Bool instead of bool.

> Source/WebKit/efl/ewk/ewk_notification.h:49
> +EAPI void ewk_notification_dispatch_click(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:56
> +EAPI void ewk_notification_dispatch_close(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:63
> +EAPI void ewk_notification_dispatch_error(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:70
> +EAPI void ewk_notification_dispatch_show(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:79
> +EAPI Eina_Stringshare* ewk_notification_title_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:82
> + * Get icon url from @c Ewk_Notification.

s/url/URL/g.

> Source/WebKit/efl/ewk/ewk_notification.h:86
> + * @return icon url of the notification which has to be deleted by
eina_stringshare_del after using it.

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:88
> +EAPI Eina_Stringshare* ewk_notification_icon_url_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:98
> +EAPI Eina_Stringshare* ewk_notification_body_get(Ewk_Notification* en);
> +

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:100
> + * Get tag/replaced id from @c Ewk_Notification.

I don't understand what does "tag/replaed" mean ?

> Source/WebKit/efl/ewk/ewk_notification.h:106
> +EAPI Eina_Stringshare* ewk_notification_tag_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:115
> +EAPI Eina_Stringshare* ewk_notification_dir_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification_private.h:28
> +WebCore::Notification* ewk_notification_get_notification(Ewk_Notification*
ewkNotification);

Naming violation. ewk_notification_notification_get(). If you worry about
duplicating naming, I think you can modify name more meaning thing.

> Source/WebKit/efl/ewk/ewk_notification_private.h:33
> +

Unneeded line.

> Source/WebKit/efl/ewk/ewk_view.cpp:4381
> +void ewk_view_notification_permissions_set(Evas_Object* ewkView, const char*
domain, Eina_Bool isAllowed)

s/isAllowed/allow/g.

> Source/WebKit/efl/ewk/ewk_view.cpp:4386
> +   
static_cast<WebCore::NotificationPresenterClientEfl*>(WebCore::NotificationCont
roller::clientFrom(priv->page.get()))->addPermission(domain, isAllowed);

ditto.

> Source/WebKit/efl/ewk/ewk_view.h:76
> + *  - "notification,show", Ewk_Notification*: request to show notification.

List this signal up by alphabetical order.

> Source/WebKit/efl/ewk/ewk_view.h:2638
> +EAPI void ewk_view_notification_permissions_set(Evas_Object* o, const char*
domain, Eina_Bool isAllowed);

Change isAllowed variable with allow. In addition, move '*' operator to
variable side.

> Source/WebKit/efl/ewk/ewk_view_private.h:158
> +WebCore::NotificationPresenterClientEfl*
ewk_view_notification_client_get(Evas_Object* ewkView);

Add *const* keyword for _get() function.