[Webkit-unassigned] [Bug 73544] [EFL] Add Web Notification feature for WebKit-EFL
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jan 10 20:34:37 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=73544
--- Comment #30 from Ryuan Choi <ryuan.choi at samsung.com> 2012-01-10 20:34:36 PST ---
(From update of attachment 121963)
View in context: https://bugs.webkit.org/attachment.cgi?id=121963&action=review
IMO, Approach looks better than before.
> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:42
> + HashSet<Ewk_Notification*>::iterator iter = m_pendingNotifications.begin();
> + while (iter != m_pendingNotifications.end())
> + ewk_notification_destroy(*iter);
I'm not sure whether we should keep and control Ewk_Notification.
> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:63
> + if (notification->isHTML()) {
> + Ewk_Notification* ewkNotification = ewk_notification_create(this, notification);
> + evas_object_smart_callback_call(m_view, "notification,htmlshow", ewkNotification);
> + m_pendingNotifications.add(ewkNotification);
> + } else {
> + Ewk_Notification* ewkNotification = ewk_notification_create(this, notification);
> + evas_object_smart_callback_call(m_view, "notification,show", ewkNotification);
> + m_pendingNotifications.add(ewkNotification);
> + }
Instead of controlling m_pendingNotifications,
why don't you call ewk_notification_deref() after calling evas_object_smart_callback_call()?
> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:71
> + evas_object_smart_callback_call(m_view, "notification,cancel", &ewkNotification);
& looks typo, right?
> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:104
> + evas_object_smart_callback_call(m_view, "notification,requestPermission"
IMO, "notification,permission,request" is better
> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:46
> + void makePermissionCache(char** domains, int size);
> + void permittedCallbackCall(const char* domain, const bool permission);
IMO, we'd better to use String and Vector for argument.
> Source/WebKit/efl/ewk/ewk_notification.cpp:32
> +struct _Ewk_Notification {
> + WebCore::NotificationPresenterClientEfl* notificationPresenter;
> + WebCore::Notification* notification;
ENABLE(NOTIFICATIONS) is missing.
> Source/WebKit/efl/ewk/ewk_notification.cpp:46
> +char* ewk_notification_icon_url_get(Ewk_Notification* ewkNotification)
How do you think about returning const char* ?
> Source/WebKit/efl/ewk/ewk_notification.cpp:111
> +void ewk_notification_permitted_domains_add(const Ewk_Notification* ewkNotification, char** domains, int size)
> +{
> +#if ENABLE(NOTIFICATIONS)
> + ewkNotification->notificationPresenter->makePermissionCache(domains, size);
> +#endif
> +}
I'm not sure, but char** looks ambiguous.
Why don't you use Eina_List ?
> Source/WebKit/efl/ewk/ewk_notification.cpp:136
> + delete ewkNotification->iconURL;
> + delete ewkNotification->title;
> + delete ewkNotification->body;
> + delete ewkNotification->htmlURL;
> + delete ewkNotification->dir;
> + delete ewkNotification;
You should not delete the memory which assigned by strdup()
> Source/WebKit/efl/ewk/ewk_notification_private.h:31
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
For the internal header, we don't need `extern "C"`
> Source/WebKit/efl/ewk/ewk_view.h:88
> + * - "notification,requestPermission", char*: requested domain for getting user permission from the user.
char* or const char* ? which one is better ?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list