[Webkit-unassigned] [Bug 73544] [EFL] Support for Web Notification.

[bugzilla-daemon at webkit.org]

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


Gyuyoung Kim <gyuyoung.kim at samsung.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #145701|review?                     |review-
               Flag|                            |




--- Comment #50 from Gyuyoung Kim <gyuyoung.kim at samsung.com>  2012-06-05 00:10:10 PST ---
(From update of attachment 145701)
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::NotificationController::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.

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