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

[bugzilla-daemon at webkit.org]

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





--- Comment #51 from Kihong Kwon <kihong.kwon at samsung.com>  2012-06-05 01:19:23 PST ---
(From update of attachment 145701)
View in context: https://bugs.webkit.org/attachment.cgi?id=145701&action=review

Thank you for your kind comments. Gyuyoung~:-)

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

OK.

>> 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 ?

There are no implementation in the chromium and qt also.
This function is for W3C web notification spec, but we are support legacy notification until now like chromium and qt.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:32
>> +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> 
> Is it better to add comment for these internal functions ?

OK.

>> Source/WebKit/efl/ewk/ewk_notification.h:38
>> +    char* domain;
> 
> Move '*' to variable side.

OK.

>> 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 ?

I will change this to replaceId.(followed by standard.)

>> 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.

OK.

>> 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.

OK.

>> Source/WebKit/efl/ewk/ewk_view.h:76
>> + *  - "notification,show", Ewk_Notification*: request to show notification.
> 
> List this signal up by alphabetical order.

OK.

>> 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.

OK.

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