[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 #14 from Grzegorz <g.czajkowski at samsung.com>  2011-12-14 00:56:55 PST ---
View in context: https://bugs.webkit.org/attachment.cgi?id=119165&action=review

> ChangeLog:3
> +        [EFL] Add Web Notification feature for Efl port

I'd rather mention that API is added in this patch and please add dot:
[EFL] Add API for Web Notification feature.

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

Missing dot.

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Add Web Notification feature for Efl port

Please change subject as I mentioned above.

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

Missing dot.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:49
> +

In my opinion this an empty line is not necessary.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:87
> +        m_cachedPermissions.add(domain);

What do you say about adding this without assign it to the String variable?
m_cachedPermissions.add(String(domains[i]));
If you agree with this suggestion, please remove unnecessary {} in for.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:94
> +

Unnecessary an empty line.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:95
> +    m_cachedPermissions.add(permittedDomain);

m_cachedPermissions.add(String(domain));

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:53
> +    Evas_Object* m_view;

I would rather change an order here. Please define members of class first then methods.

> Source/WebKit/efl/ewk/ewk_notification.cpp:101
> +

Unnecessary an empty line.

> Source/WebKit/efl/ewk/ewk_notification.cpp:108
> +

Ditto.

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

Please add doxygen section here:
/**
 * @file ....
 * @brief ...
 *
 * Detailed description.
 */

> Source/WebKit/efl/ewk/ewk_notification.h:32
> +typedef struct _Ewk_Notification Ewk_Notification;

Generally WebKit-Efl documents typedefs too.

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

Please describe these struct members.

> Source/WebKit/efl/ewk/ewk_notification.h:43
> + * Call a callback fucntion which is resisterd, after receiving a user approval.

Call -> Calls
resisterd -> registered

> Source/WebKit/efl/ewk/ewk_notification.h:45
> + * @param ewkView view to receive message from browser

@param ewkView view to call registered function

> Source/WebKit/efl/ewk/ewk_notification.h:46
> + * @param permitted whether permit a notificaion or not by user

* @param permitted @c EINA_TRUE to allow (what? notificaion or callbakc), @c EINA_FALSE to ...

> Source/WebKit/efl/ewk/ewk_notification.h:53
> + * Make a cacheList for permitted domains

Make -> Makes
Please do not use variable names here.
Missing dot.
Please add en empty line here to signal detailed description.

> Source/WebKit/efl/ewk/ewk_notification.h:54
> + * This function is called when initializing a browser application.

This comment confuses developer. As I understand your intention  this API should be called if application wants to has some benefits of it.
And please do not user "browser" in comments. Many applications use WebKit not only browser.

> Source/WebKit/efl/ewk/ewk_notification.h:56
> + * @param ewkView view to receive message from browser

to receive message from browser -> to add permitted domains

> Source/WebKit/efl/ewk/ewk_notification.h:57
> + * @param domains domain list which is added to permission cache list

* @param domains domain list which is added - please describe what benefits are from adding these (notification?)

> Source/WebKit/efl/ewk/ewk_notification.h:64
> + * Make an ondisplay callback call for JavaScript after notification is displayed

Calls ondisplay callback ... ?

> Source/WebKit/efl/ewk/ewk_notification.h:66
> + * @param ewkView view to receive message from browser

Ditto.

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

This section should be changed regarding to the previous comments.

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

Ditto.

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

Ditto.

> Tools/ChangeLog:3
> +        [EFL] Add Web Notification feature for Efl port

Please change a subject.

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

Missing dot.

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