[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 #38 from Kihong Kwon <kihong.kwon at samsung.com>  2012-01-25 03:39:04 PST ---
(From update of attachment 123901)
View in context: https://bugs.webkit.org/attachment.cgi?id=123901&action=review

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

I think so.
The permission request has to obtain a user's confirmation immediately before the user performs other operations on that page.

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

You are right.

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

OK.

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

You already mentioned about this.
Not a problem.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:155
>> +            ->dir().utf8().data());
> 
> There are many issues.
> you don't need to cast, right?
> 
> 139 and 140 lines is wrong.

Yes. There are no needs to cast.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:183
>> +#if ENABLE(NOTIFICATIONS)
> 
> empty line please

OK.

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