Bug 90603

Summary: [EFL] Support the permission function of the Web Notification.
Product: WebKit Reporter: Kihong Kwon <kihong.kwon>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: cdumez, donggwan.kim, gyuyoung.kim, gyuyoung.kim, haraken, jinwoo7.song, kenneth, lucas.de.marchi, mcatanzaro, rakuco, tmpsantos, vimff0, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88919, 90936    
Bug Blocks: 73544, 95925    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
gyuyoung.kim: review-, gyuyoung.kim: commit-queue-
Patch
none
Patch
none
Patch
none
Patch none

Description Kihong Kwon 2012-07-05 05:09:18 PDT
Implement checkPermission and requestPermission of Web Notification.
Comment 1 Kihong Kwon 2012-07-06 01:04:50 PDT
Created attachment 151029 [details]
Patch
Comment 2 Gyuyoung Kim 2012-07-08 22:07:31 PDT
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.
Comment 3 Kihong Kwon 2012-07-08 22:31:45 PDT
(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.
Comment 4 Chris Dumez 2012-07-08 22:41:11 PDT
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.
Comment 5 Kihong Kwon 2012-07-08 23:44:12 PDT
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.
Comment 6 Kihong Kwon 2012-07-09 00:33:44 PDT
Created attachment 151199 [details]
Patch
Comment 7 Hajime Morrita 2012-07-09 22:20:18 PDT
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.
Comment 8 Kihong Kwon 2012-07-10 03:25:36 PDT
(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. :)
Comment 9 Kenneth Rohde Christiansen 2012-07-12 20:25:35 PDT
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.
Comment 10 Gyuyoung Kim 2012-07-14 04:51:37 PDT
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)
Comment 11 Kihong Kwon 2012-07-15 23:58:04 PDT
(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. :)
Comment 12 Kihong Kwon 2012-07-16 00:00:38 PDT
(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.
Comment 13 Kenneth Rohde Christiansen 2012-07-16 20:43:22 PDT
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.
Comment 14 Kihong Kwon 2012-07-17 18:26:20 PDT
(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~:)
Comment 15 Kihong Kwon 2012-07-17 19:19:21 PDT
Created attachment 152903 [details]
Patch
Comment 16 Kenneth Rohde Christiansen 2012-07-17 20:03:48 PDT
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 :-(
Comment 17 Kihong Kwon 2012-07-17 21:51:58 PDT
(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.
Comment 18 Kihong Kwon 2012-07-22 20:19:42 PDT
Created attachment 153724 [details]
Patch
Comment 19 Gyuyoung Kim 2012-07-23 23:37:12 PDT
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.
Comment 20 Kentaro Hara 2012-07-24 00:52:32 PDT
Just FYI, Notification.permissionLevel() is going to be changed to Notification.permission() (bug 88919)
Comment 21 Kihong Kwon 2012-08-26 19:42:57 PDT
Created attachment 160621 [details]
Patch
Comment 22 Kenneth Rohde Christiansen 2012-08-27 01:08:17 PDT
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?
Comment 23 Mikhail Pozdnyakov 2012-08-27 01:22:55 PDT
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?
Comment 24 Gyuyoung Kim 2012-08-27 01:23:58 PDT
Comment on attachment 160621 [details]
Patch

We started to add unit test for public API as well. Please add unit test.
Comment 25 Gyuyoung Kim 2012-08-27 18:40:06 PDT
Comment on attachment 160621 [details]
Patch

Set r- because of many comments.
Comment 26 Chris Dumez 2012-09-04 23:00:57 PDT
Any update on this?
Comment 27 Kihong Kwon 2012-09-04 23:25:44 PDT
(In reply to comment #26)
> Any update on this?

I will update this soon. :)
Comment 28 Kihong Kwon 2012-09-05 04:49:21 PDT
(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. :)
Comment 29 Kihong Kwon 2012-09-05 19:14:55 PDT
(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.
Comment 30 Kihong Kwon 2012-09-05 19:17:05 PDT
(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.
Comment 31 Gyuyoung Kim 2012-09-05 19:20:06 PDT
(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.
Comment 32 Kihong Kwon 2012-09-05 20:18:58 PDT
Created attachment 162401 [details]
Patch
Comment 33 Gyuyoung Kim 2012-09-05 20:26:49 PDT
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.
Comment 34 Kihong Kwon 2012-09-05 20:34:47 PDT
(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.
Comment 35 Kihong Kwon 2012-09-05 21:47:09 PDT
Created attachment 162411 [details]
Patch
Comment 36 Gyuyoung Kim 2012-09-06 02:03:32 PDT
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.
Comment 37 Gyuyoung Kim 2012-09-06 02:04:17 PDT
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 ?
Comment 38 Kihong Kwon 2012-09-06 03:59:14 PDT
Created attachment 162471 [details]
Patch
Comment 39 Kenneth Rohde Christiansen 2012-09-06 06:33:27 PDT
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
Comment 40 Kihong Kwon 2012-09-06 07:37:53 PDT
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()).
Comment 41 Gyuyoung Kim 2012-09-06 18:06:25 PDT
(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.
Comment 42 Kihong Kwon 2012-09-06 20:07:16 PDT
Created attachment 162660 [details]
Patch
Comment 43 WebKit Review Bot 2012-09-07 00:55:44 PDT
Comment on attachment 162660 [details]
Patch

Clearing flags on attachment: 162660

Committed r127839: <http://trac.webkit.org/changeset/127839>
Comment 44 WebKit Review Bot 2012-09-07 00:55:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 45 Kenneth Rohde Christiansen 2012-09-07 00:57:55 PDT
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.
Comment 46 Kenneth Rohde Christiansen 2012-09-07 02:10:58 PDT
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 ?
Comment 47 Gyuyoung Kim 2012-09-07 02:59:42 PDT
Reverted r127839 for reason:

r127839

Committed r127848: <http://trac.webkit.org/changeset/127848>
Comment 48 Michael Catanzaro 2017-03-11 10:34:43 PST
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.