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

[bugzilla-daemon at webkit.org]

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





--- Comment #33 from Kihong Kwon <kihong.kwon at samsung.com>  2012-01-11 03:27:28 PST ---
(In reply to comment #30)
> (From update of attachment 121963 [details])
> 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.

I think we have to keep Ewk_Notificaiton for memory management.
If app doesn't send a (onclose/onerror) event and doesn't call a deref, there can be memory leak.

> 
> > 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?
Yes.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:104
> > +        evas_object_smart_callback_call(m_view, "notification,requestPermission"
> 
> IMO, "notification,permission,request" is better
I agree.
> 
> > 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.
OK.
> 
> > 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* ?
I think so.
> 
> > 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 ?
OK.
> 
> > 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()
We can use delete with strdup(), but I will change strdup to eina_stringshare_add().
> 
> > Source/WebKit/efl/ewk/ewk_notification_private.h:31
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> 
> For the internal header, we don't need `extern "C"`
OK.
> 
> > 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 ?
we can not use a const char* in the evas_object_smart_callback_call.
In the evas_object_smart_callback_call receive a void* parameter.

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