[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 #36 from Ryuan Choi <ryuan.choi at samsung.com>  2012-01-25 02:36:09 PST ---
(From update of attachment 123901)
View in context: https://bugs.webkit.org/attachment.cgi?id=123901&action=review

r- on my side.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:103
> +    // Because we are using modal dialogs to request permission, it's impossible to cancel them.

Should we implement this as modal popup?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:106
> +void NotificationPresenterClientEfl::makePermissionCache(Vector<String> domains)

const Vector<String>& looks enough.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:114
> +    for (size_t i = 0; i < m_callbacks.size(); i++) {

{ looks wrong.

> Source/WebKit/efl/ewk/ewk_notification.cpp:63
> +    return ewkNotification->iconURL;

IIRC, iconURL may have trash because you assign values only when icon string is not empty.

> Source/WebKit/efl/ewk/ewk_notification.cpp:155
> +    if (static_cast<WebCore::Notification*>(notification)->replaceId().length())
> +        ewkNotification->iconURL = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->replaceId().utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->contents().icon.string().length())
> +        ewkNotification->iconURL = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->contents().icon.string().utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->contents().title.length())
> +        ewkNotification->title = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->contents().title.utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->contents().body.length())
> +        ewkNotification->body = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->contents().body.utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->url().string().length())
> +        ewkNotification->url = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->url().string().utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->dir().length())
> +        ewkNotification->dir = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->dir().utf8().data());

There are many issues.
you don't need to cast, right?

139 and 140 lines is wrong.

> Source/WebKit/efl/ewk/ewk_notification.cpp:183
> +        return;
> +#if ENABLE(NOTIFICATIONS)

empty line please

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