[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 #4 from Christophe Dumez <christophe.dumez at intel.com>  2012-07-08 22:41:11 PST ---
(From update of attachment 151029)
View in context: https://bugs.webkit.org/attachment.cgi?id=151029&action=review

> Source/WebKit/efl/ChangeLog:9
> +        Stored permissions in the outside application(like a browser) can be cached by ewk_view_notification_permissions_set.

"can be cached using ewk_..."

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:93
> +    String domain = context->url().host();

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

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:107
> +        m_pendingPermissionRequests.remove(domain);

Ditto.

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

String::fromUTF8() ?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:131
> +    m_cachedPermissions.add(String(domain), isAllowed);

Ditto.

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

!!allow ?

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

> 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();

>> Source/WebKit/efl/ewk/ewk_view.cpp:4526
>> +    delete requestedDomain;
> 
> Isn't free() better ? Because, strdup uses malloc.

Gyuyoung is right but this becomes useless if you use CString.

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

> 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();

> Source/WebKit/efl/ewk/ewk_view.h:78
> + *  - "notification,permission,request", const char*: request to confirm permission for domain from uesr.

"user"

> Source/WebKit/efl/ewk/ewk_view.h:2699
> + * Set cache by stored permission list for notification.

This sentence is very difficult to understand.

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

"domain string used to determine the permission" ?

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

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

const reference.

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

Ditto.

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