[Webkit-unassigned] [Bug 73544] [EFL] Add Web Notification feature for Efl port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73544





--- Comment #18 from Grzegorz <g.czajkowski at samsung.com>  2011-12-15 01:09:18 PST ---
Thanks for your changes Kihong.
I did a review again and some fixes are still needed especially in doxygen comments. Please see other ewk API documentation and try to adjust your comments.
Please consider changing API name. I suggest you to notify this API by WebKit-EFL list.


View in context: https://bugs.webkit.org/attachment.cgi?id=119191&action=review

> ChangeLog:6
> +        This is an implementation about web notification for efl.

I would rather remove this general description from here and add what you really change. For example:
Enables Web Notification feature for WebKit-Efl.

> ChangeLog:8
> +        * Source/cmake/OptionsEfl.cmake: Enable ENABLE_NOTIFICATIONS feture

1) If you agree with my previous comment this an additional description isn't necessary.
2) I don't see your modifications in Source/cmake/OptionsEfl.cmake but in Source/WebKit/efl/CMakeListsEfl.txt. Please verify ChangeLog.

> Source/WebKit/efl/ChangeLog:8
> +        This is an implementation about web notification for efl.

This description needs changes, what do you say about this?
Implements Web Notification feature and exposes API for WebKit-EFl.
This an implementation is based on webkitNotificaion property in window.navigator.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> +        return PermissionDenied;

This value is also returned in previous condition. Please consider to merger them into one, for example

if (!m_cachedPermissions.size()
    || m_cachedPermissions.find(context->securityOrigin()->toString()) == m_cachedPermissions.end())
    return PermissionDenied;

> Source/WebKit/efl/ewk/ewk_notification.cpp:62
> +    if (!permit)

I would rather reverse logic and call callback if permit is true.

> Source/WebKit/efl/ewk/ewk_notification.h:22
> + * @brief APIs for the web notification of WebKit-EFL

Please describe all signals which are emitted by this API, for example notification,contents,show, notification,contents,htmlshow etc.

> Source/WebKit/efl/ewk/ewk_notification.h:39
> + */

/// Creates a type name for @c _Ewk_Notification.

> Source/WebKit/efl/ewk/ewk_notification.h:41
> +

/// Contains Web Notification data.

> Source/WebKit/efl/ewk/ewk_notification.h:43
> +    void* notification;

Should user have access to this member? In my opinion it should be moved to an implementation file.

> Source/WebKit/efl/ewk/ewk_notification.h:44
> +    char* iconURL;

Please describe this variable because it's accessible from API.

> Source/WebKit/efl/ewk/ewk_notification.h:45
> +    char* title;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:46
> +    char* body;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:47
> +    char* url;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:51
> + * Calls a callback fucntion which is registerd, after receiving a user approval.

It would be nice to describe what the function does for caller.
Please describe what benefits are when this callback is called.

> Source/WebKit/efl/ewk/ewk_notification.h:53
> + * @param ewkView view to call registered function

This comment rather notify caller that he should register function. But it registered by WebCore. Am I right?

> Source/WebKit/efl/ewk/ewk_notification.h:54
> + * @param permitted @c EINA_TRUE to allow to show notificaion, @c EINA_FALSE to not allow to show notificaiton.

1) notificaion -> notification
2) notificaiton -> notification

> Source/WebKit/efl/ewk/ewk_notification.h:56
> + *

Unnecessary an empty line.

> Source/WebKit/efl/ewk/ewk_notification.h:58
> +EAPI void ewk_notification_permission_callback(const Evas_Object *o, Eina_Bool permitted, const char* domain);

This API name should be renamed, please use verb in API name, for example:
wk_notification_permission_callback -> wk_notification_permission_callback_add ?

> Source/WebKit/efl/ewk/ewk_notification.h:61
> + * Makes a Cache for permitted domains

Missing dot.

> Source/WebKit/efl/ewk/ewk_notification.h:63
> + * This function can be called at initializing time if a application have permmited domains.

permmited -> permitted

> Source/WebKit/efl/ewk/ewk_notification.h:71
> +EAPI void ewk_notification_add_permitted_domains(const Evas_Object *o, char** domains, int size);

ewk_notification_add_permitted_domains -> ewk_notification_permitted_domains_add ?

> Tools/ChangeLog:6
> +        This is an implementation about web notification for efl.

I would rather remove this general description from here and add what you really change

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.