[Webkit-unassigned] [Bug 90603] [EFL] Support the permission function of the Web Notification.

[bugzilla-daemon at webkit.org]

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





--- Comment #5 from Kihong Kwon <kihong.kwon at samsung.com>  2012-07-08 23:44:12 PST ---
(From update of attachment 151029)
View in context: https://bugs.webkit.org/attachment.cgi?id=151029&action=review

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:93
>> +    String domain = context->url().host();
> 
> #if ENABLE(LEGACY_NOTIFICATIONS) || ENABLE(NOTIFICATIONS) ?

I don't think so.
This file is already wrapped with "#if ENABLE(LEGACY_NOTIFICATIONS) || ENABLE(NOTIFICATIONS)".

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:99
>> +        m_pendingLegacyPermissionRequests.remove(domain);
> 
> Since you already have the iterator you could call HashMap::remove(iterator) which is likely faster than HashMap::remove(key).

You are right.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:115
>> +    LegacyPermissionRequestMap::iterator legacyIter = m_pendingLegacyPermissionRequests.find(String(domain));
> 
> String::fromUTF8() ?

I'm not sure about changing this?
Do you have a reason for that?

>> Source/WebKit/efl/ewk/ewk_view.cpp:4517
>> +    static_cast<WebCore::NotificationPresenterClientEfl*>(WebCore::NotificationController::clientFrom(priv->page.get()))->addPermission(domain, allow);
> 
> !!allow ?

It will be changed to allowed.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4522
>> +void ewk_view_notification_permission_request(Evas_Object* ewkView, WTF::String& domain)
> 
> Looks like domain reference should be const.

OK.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4524
>> +    char* requestedDomain = strdup(domain.utf8().data());
> 
> Instead of using strdup and manage memory yourself I would prefer if you use CString. E.g. CString requestedDomain = domain.utf8();

OK, It's better than mine. :)

>> Source/WebKit/efl/ewk/ewk_view.cpp:4529
>> +void ewk_view_notification_permission_cancel(Evas_Object* ewkView, WTF::String& domain)
> 
> Looks like domain reference should be const.

OK.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4531
>> +    char* requestedDomain = strdup(domain.utf8().data());
> 
> Instead of using strdup and manage memory yourself I would prefer if you use CString. E.g. CString requestedDomain = domain.utf8();

ditto.

>> Source/WebKit/efl/ewk/ewk_view.h:2699
>> + * Set cache by stored permission list for notification.
> 
> This sentence is very difficult to understand.

I will change description.

>> Source/WebKit/efl/ewk/ewk_view.h:2702
>> + * @param domain string for determine permission.
> 
> "domain string used to determine the permission" ?

Fixed.

>> Source/WebKit/efl/ewk/ewk_view.h:2705
>> +EAPI void ewk_view_notification_permissions_set(Evas_Object *o, const char *domain, Eina_Bool allow);
> 
> Shouldn't this return a boolean like the other Ewk setters?

Fixed.

>> Source/WebKit/efl/ewk/ewk_view_private.h:160
>> +void ewk_view_notification_permission_request(Evas_Object* ewkView, WTF::String& domain);
> 
> const reference.

Fixed.

>> Source/WebKit/efl/ewk/ewk_view_private.h:161
>> +void ewk_view_notification_permission_cancel(Evas_Object* ewkView, WTF::String& domain);
> 
> Ditto.

Fixed.

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