Bug 90603 – [EFL] Support the permission function of the Web Notification.

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Kihong Kwon]

Implement checkPermission and requestPermission of Web Notification.

[Kihong Kwon]

Created attachment 151029 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 151029 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151029&action=review

> Source/WebKit/efl/ewk/ewk_view.cpp:4526
> +    delete requestedDomain;

Isn't free() better ? Because, strdup uses malloc.

> Source/WebKit/efl/ewk/ewk_view.cpp:4533
> +    delete requestedDomain;

ditto.

[Kihong Kwon]

(In reply to comment #2)
> (From update of attachment 151029 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151029&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4526
> > +    delete requestedDomain;
> 
> Isn't free() better ? Because, strdup uses malloc.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4533
> > +    delete requestedDomain;
> 
> ditto.

OK, I will fix them in next patch. Thanks.

[Chris Dumez]

Comment on attachment 151029 [details]
Patch

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.

[Kihong Kwon]

Comment on attachment 151029 [details]
Patch

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.

[Kihong Kwon]

Created attachment 151199 [details]
Patch

[Hajime Morrita]

Comment on attachment 151199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->url().host());

Is it intentional to use host()? For example, is it OK to ignore the port number?
It looks WebKit2/mac uses SecurityOrigin for that same purpose.

[Kihong Kwon]

(In reply to comment #7)
> (From update of attachment 151199 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> > +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->url().host());
> 
> Is it intentional to use host()? For example, is it OK to ignore the port number?
> It looks WebKit2/mac uses SecurityOrigin for that same purpose.

You are right.
I think I need to change about that.
Thank you. :)

[Kenneth Rohde Christiansen]

Comment on attachment 151199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review

>>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
>>> +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->url().host());
>> 
>> Is it intentional to use host()? For example, is it OK to ignore the port number?
>> It looks WebKit2/mac uses SecurityOrigin for that same purpose.
> 
> You are right.
> I think I need to change about that.
> Thank you. :)

You could take a look at how we are doing this for Qt (I am not sure the patch was ever landed but one exists in bugzilla).

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

Why support legacy notifications at all? They should go away no?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:41
> +#if ENABLE(LEGACY_NOTIFICATIONS)

I would really let EFL just not support legacy notifications. You are adding much code and it makes it even harder removing the feature later.

[Gyuyoung Kim]

Comment on attachment 151199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:34
> +    NotificationPresenterClientEfl(Evas_Object* view);

Use *explicit* keyword for constructor which has a parameter.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:41
>> +#if ENABLE(LEGACY_NOTIFICATIONS)
> 
> I would really let EFL just not support legacy notifications. You are adding much code and it makes it even harder removing the feature later.

According to my quick search, it looks other ports are supporting LEGACY_NOTIFICATIONS. But, there is no reason to use LEGACY_NOTIFICATIONS on EFL port, I also think we don't need to support it.

> Source/WebKit/efl/ewk/ewk_view.cpp:4515
> +    EWK_VIEW_SD_GET(ewkView, smartData);

Use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false) instead of this.

> Source/WebKit/efl/ewk/ewk_view.cpp:4516
> +    EWK_VIEW_PRIV_GET(smartData, priv);

Use EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false)

[Kihong Kwon]

(In reply to comment #9)
> (From update of attachment 151199 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review
> 
> >>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> >>> +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->url().host());
> >> 
> >> Is it intentional to use host()? For example, is it OK to ignore the port number?
> >> It looks WebKit2/mac uses SecurityOrigin for that same purpose.
> > 
> > You are right.
> > I think I need to change about that.
> > Thank you. :)
> 
> You could take a look at how we are doing this for Qt (I am not sure the patch was ever landed but one exists in bugzilla).

OK. I'll do that, thanks~ :)
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:115
> > +#if ENABLE(LEGACY_NOTIFICATIONS)
> > +    LegacyPermissionRequestMap::iterator legacyIter = m_pendingLegacyPermissionRequests.find(String(domain));
> 
> Why support legacy notifications at all? They should go away no?
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:41
> > +#if ENABLE(LEGACY_NOTIFICATIONS)
> 
> I would really let EFL just not support legacy notifications. You are adding much code and it makes it even harder removing the feature later.

I agree with you on one hand but almost sites support legacy notification until now on the other. (html5test.com, html5rocks.com...)
Actually I haven't seen yet a site support W3C "Web Notifications".

In addition, even firefox supports legacy notifications with this link (http://www.chromium.org/developers/design-documents/desktop-notifications/api-specification)

That's why I would like to add LEGACY_NOTIFICATIONS.

I would like to get your opinion about this.
Thank you. :)

[Kihong Kwon]

(In reply to comment #10)
> (From update of attachment 151199 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:34
> > +    NotificationPresenterClientEfl(Evas_Object* view);
> 
> Use *explicit* keyword for constructor which has a parameter.
OK.
> 
> >> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:41
> >> +#if ENABLE(LEGACY_NOTIFICATIONS)
> > 
> > I would really let EFL just not support legacy notifications. You are adding much code and it makes it even harder removing the feature later.
> 
> According to my quick search, it looks other ports are supporting LEGACY_NOTIFICATIONS. But, there is no reason to use LEGACY_NOTIFICATIONS on EFL port, I also think we don't need to support it.

I add my answer for this to reply of Kenneth's comments.
Please read it and leave comments.

> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4515
> > +    EWK_VIEW_SD_GET(ewkView, smartData);
> 
> Use EWK_VIEW_SD_GET_OR_RETURN(ewkView, smartData, false) instead of this.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4516
> > +    EWK_VIEW_PRIV_GET(smartData, priv);
> 
> Use EWK_VIEW_PRIV_GET_OR_RETURN(smartData, priv, false)
OK, Thanks.

[Kenneth Rohde Christiansen]

Comment on attachment 151199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review

>>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:115
>>> +    LegacyPermissionRequestMap::iterator legacyIter = m_pendingLegacyPermissionRequests.find(String(domain));
>> 
>> Why support legacy notifications at all? They should go away no?
> 
> I agree with you on one hand but almost sites support legacy notification until now on the other. (html5test.com, html5rocks.com...)
> Actually I haven't seen yet a site support W3C "Web Notifications".
> 
> In addition, even firefox supports legacy notifications with this link (http://www.chromium.org/developers/design-documents/desktop-notifications/api-specification)
> 
> That's why I would like to add LEGACY_NOTIFICATIONS.
> 
> I would like to get your opinion about this.
> Thank you. :)

I am pretty sure that Safari on Desktop and iOS doesnt support legacy notifications, also I don't think Internet Explorer nor Opera does so. It is a dead fish in the water. They became legacy notifications as other vendors had issues with the old spec.

[Kihong Kwon]

(In reply to comment #13)
> (From update of attachment 151199 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=151199&action=review
> 
> I am pretty sure that Safari on Desktop and iOS doesnt support legacy notifications, also I don't think Internet Explorer nor Opera does so. It is a dead fish in the water. They became legacy notifications as other vendors had issues with the old spec.

I accept your opinion.
I will update this patch without LEGACY_NOTIFICATIONS.
Thank you for your comments~:)

[Kihong Kwon]

Created attachment 152903 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 152903 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=152903&action=review

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->securityOrigin()->toString());
> +
> +    if (iter == m_cachedPermissions.end())
> +        return PermissionNotAllowed;
> +
> +    if (iter->second)
> +        return PermissionAllowed;
> +
>      return PermissionDenied;
>  }

So this will never reach the last return? What is the difference between Denied and NotAllowed?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
> +void NotificationPresenterClientEfl::addPermissionCache(const char* domain, const bool isAllowed)

addTo ?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:89
> +    PermissionsMap::iterator iter = m_cachedPermissions.find(String(domain));
> +
> +    if (iter == m_cachedPermissions.end())
> +        return;
> +
> +    m_cachedPermissions.add(String(domain), isAllowed);

So you check whehter is is already cached. If it is not you return, thus never cache :-(

[Kihong Kwon]

(In reply to comment #16)
> (From update of attachment 152903 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152903&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> > +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->securityOrigin()->toString());
> > +
> > +    if (iter == m_cachedPermissions.end())
> > +        return PermissionNotAllowed;
> > +
> > +    if (iter->second)
> > +        return PermissionAllowed;
> > +
> >      return PermissionDenied;
> >  }
> 
> So this will never reach the last return? What is the difference between Denied and NotAllowed?

Denied means user already select deny when requestPermission has been called.
But, NotAllowed means there haven't been requested to the user yet, in this case requestPermission need to be called.

Last return statement can be called when iter->second(Permission allowed) is false.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
> > +void NotificationPresenterClientEfl::addPermissionCache(const char* domain, const bool isAllowed)
> 
> addTo ?
OK. I'will change it.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:89
> > +    PermissionsMap::iterator iter = m_cachedPermissions.find(String(domain));
> > +
> > +    if (iter == m_cachedPermissions.end())
> > +        return;
> > +
> > +    m_cachedPermissions.add(String(domain), isAllowed);
> 
> So you check whehter is is already cached. If it is not you return, thus never cache :-(

I don't think so, permission cache is added in the requestPermission.
If we want to use notification, we need to call requestPermission to get a permission.

[Kihong Kwon]

Created attachment 153724 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 153724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=153724&action=review

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

Use String::fromUTF8() instead of String().

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:101
> +    m_cachedPermissions.add(String(ewk_security_origin_string_get(origin)), isAllowed);

ditto.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:34
> +    NotificationPresenterClientEfl(Evas_Object* view);

Missing *explicit* keyword.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:48
> +    void addToPermissionCache(const char* domain, bool isAllowed);

Use String class in WebCoreSupport layer.

> Source/WebKit/efl/ewk/ewk_view.cpp:110
> +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)

Don't you remove LEGACY_NOTIFICATIONS macro in this patch ?

> Source/WebKit/efl/ewk/ewk_view.cpp:773
> +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:4520
> +    EWK_VIEW_SD_GET(ewkView, smartData);

Use EWK_VIEW_SD_GET_OR_RETURN.

> Source/WebKit/efl/ewk/ewk_view.cpp:4521
> +    EWK_VIEW_PRIV_GET(smartData, priv);

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:4532
> +    EWK_VIEW_SD_GET(ewkView, smartData);

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:4533
> +    EWK_VIEW_PRIV_GET(smartData, priv);

ditto.

[Kentaro Hara]

Just FYI, Notification.permissionLevel() is going to be changed to Notification.permission() (bug 88919)

[Kihong Kwon]

Created attachment 160621 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 160621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160621&action=review

> Source/WebKit/efl/ewk/ewk_view.h:2699
> + * Add permitted or denied list to the permission cache for Web Notificaitons.

spelling mistakes

> Source/WebKit/efl/ewk/ewk_view.h:2710
> +Eina_Bool ewk_view_notification_permission_cache_add(Evas_Object *o, const char *domain, Eina_Bool permitted);

I think cache is a pretty overloaded name... What about "store". Will there be different permission stores? like for geolocation, notifications etc?

[Mikhail Pozdnyakov]

Comment on attachment 160621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160621&action=review

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:33
> +    : m_view(view)

Check for null?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:64
> +    Ewk_Security_Origin* origin = ewk_security_origin_new(context->securityOrigin());

shouldn't we check args for null here before usage?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> +void NotificationPresenterClientEfl::addToPermissionCache(const String domain, const bool isAllowed)

const String& domain

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:84
> +        return;

if (iter != m_cachedPermissions.end())
   m_cachedPermissions.add(domain, isAllowed);

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:90
> +    PermissionRequestMap::iterator iter = m_pendingPermissionRequests.find(const_cast<Ewk_Security_Origin*>(origin));

maybe typedef HashMap<const Ewk_Security_Origin*, RefPtr<WebCore::NotificationPermissionCallback> > PermissionRequestMap; so that you do not need const_cast?

[Gyuyoung Kim]

Comment on attachment 160621 [details]
Patch

We started to add unit test for public API as well. Please add unit test.

[Gyuyoung Kim]

Comment on attachment 160621 [details]
Patch

Set r- because of many comments.

[Chris Dumez]

Any update on this?

[Kihong Kwon]

(In reply to comment #26)
> Any update on this?

I will update this soon. :)

[Kihong Kwon]

(In reply to comment #22)
> (From update of attachment 160621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160621&action=review
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2699
> > + * Add permitted or denied list to the permission cache for Web Notificaitons.
> 
> spelling mistakes
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2710
> > +Eina_Bool ewk_view_notification_permission_cache_add(Evas_Object *o, const char *domain, Eina_Bool permitted);
> 
> I think cache is a pretty overloaded name... What about "store". Will there be different permission stores? like for geolocation, notifications etc?

I'll change it like your recommendation.
Thanks. :)

[Kihong Kwon]

(In reply to comment #23)
> (From update of attachment 160621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160621&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:33
> > +    : m_view(view)
> 
> Check for null?
OK.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:64
> > +    Ewk_Security_Origin* origin = ewk_security_origin_new(context->securityOrigin());
> 
> shouldn't we check args for null here before usage?
Not exactly. context is already checked in the Notification::requestPermission.
Therefore we don't need check it here.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:80
> > +void NotificationPresenterClientEfl::addToPermissionCache(const String domain, const bool isAllowed)
> 
> const String& domain
You are right.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:84
> > +        return;
> 
> if (iter != m_cachedPermissions.end())
>    m_cachedPermissions.add(domain, isAllowed);
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:90
> > +    PermissionRequestMap::iterator iter = m_pendingPermissionRequests.find(const_cast<Ewk_Security_Origin*>(origin));
> 
> maybe typedef HashMap<const Ewk_Security_Origin*, RefPtr<WebCore::NotificationPermissionCallback> > PermissionRequestMap; so that you do not need const_cast?
OK. It's better.

[Kihong Kwon]

(In reply to comment #24)
> (From update of attachment 160621 [details])
> We started to add unit test for public API as well. Please add unit test.
I knew that. We need many test cases for the Web notification.
Therefore I'll add test cases after adding all APIs about Web Notification.

[Gyuyoung Kim]

(In reply to comment #30)
> (In reply to comment #24)
> > (From update of attachment 160621 [details] [details])
> > We started to add unit test for public API as well. Please add unit test.
> I knew that. We need many test cases for the Web notification.
> Therefore I'll add test cases after adding all APIs about Web Notification.

If possible, I would like to recommend to add unit test together. But, if you don't want to add unit test together with this patch. Please file a bug and link this bug.

[Kihong Kwon]

Created attachment 162401 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 162401 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162401&action=review

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
> +void NotificationPresenterClientEfl::addToPermissionCache(const String& domain, const bool isAllowed)

As parameter name, *allowed* is more used.

[Kihong Kwon]

(In reply to comment #33)
> (From update of attachment 162401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162401&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
> > +void NotificationPresenterClientEfl::addToPermissionCache(const String& domain, const bool isAllowed)
> 
> As parameter name, *allowed* is more used.
OK.

[Kihong Kwon]

Created attachment 162411 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 162411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162411&action=review

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

isAllowed -> allowed ?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:96
> +    iter->second->handleEvent(Notification::permissionString(isAllowed ? NotificationClient::PermissionAllowed : NotificationClient::PermissionDenied));

ditto.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:98
> +    m_cachedPermissions.add(String::fromUTF8(ewk_security_origin_string_get(origin)), isAllowed);

ditto.

[Gyuyoung Kim]

Comment on attachment 162411 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162411&action=review

> Source/WebKit/efl/ewk/ewk_view.h:2771
> + * @param allow @c EINA_TRUE to permit, @c EINA_FALSE to deny.

allow -> permitted ?

[Kihong Kwon]

Created attachment 162471 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 162471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162471&action=review

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:73
> +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->securityOrigin()->toString());

We normally use 'it'

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
> +void NotificationPresenterClientEfl::addToPermissionCache(const String& domain, const bool permitted)

isPermitted

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:87
> +    PermissionsMap::iterator iter = m_cachedPermissions.find(domain);
> +    if (iter == m_cachedPermissions.end())
> +        return;
> +    m_cachedPermissions.add(domain, permitted);

That cannot be right, it never gets added

[Kihong Kwon]

Comment on attachment 162471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162471&action=review

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:73
>> +    PermissionsMap::iterator iter = m_cachedPermissions.find(context->securityOrigin()->toString());
> 
> We normally use 'it'

OK. I will change all iter to it.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
>> +void NotificationPresenterClientEfl::addToPermissionCache(const String& domain, const bool permitted)
> 
> isPermitted

gyuyoung how do you think about this?
I accepted gyuyoung's opinion about this.
isPermitted? or permitted? :-)

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:87
>> +    m_cachedPermissions.add(domain, permitted);
> 
> That cannot be right, it never gets added

You are right.
I need to change this to (iter != m_cachedPermissions.end()).

[Gyuyoung Kim]

(In reply to comment #40)
 
> >> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:82
> >> +void NotificationPresenterClientEfl::addToPermissionCache(const String& domain, const bool permitted)
> > 
> > isPermitted

I think that *isPermitted* is better than *permitted* for internal function according to WebKit coding style.

[Kihong Kwon]

Created attachment 162660 [details]
Patch

[WebKit Review Bot]

Comment on attachment 162660 [details]
Patch

Clearing flags on attachment: 162660

Committed r127839: <http://trac.webkit.org/changeset/127839>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

Comment on attachment 162660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162660&action=review

removing cq- for now

> Source/WebKit/efl/ewk/ewk_view.h:2762
> +Eina_Bool ewk_view_notification_permission_store(Evas_Object *o, const char *domain, Eina_Bool permitted);

Shouldn't this be permission_store_add? Like the add is missing?

> Source/WebKit/efl/ewk/ewk_view.h:2775
> +EAPI Eina_Bool ewk_view_notification_permissions_set(Evas_Object *o, const Ewk_Security_Origin *origin, Eina_Bool permitted);

I dont get the different between this one and the above API function.

[Kenneth Rohde Christiansen]

Comment on attachment 162660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162660&action=review

> Source/WebKit/efl/ewk/ewk_view.h:2765
> + * Set permitted or denied lists to the permission cache for the Web Notifications.

This comment seems wrong

Accept or deny a permission request.

ewk_viewport_notification_permission_request_set ?

[Gyuyoung Kim]

Reverted r127839 for reason:

r127839

Committed r127848: <http://trac.webkit.org/changeset/127848>

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.