Implement checkPermission and requestPermission of Web Notification.
Created attachment 151029 [details] Patch
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.
(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 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 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.
Created attachment 151199 [details] Patch
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.
(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 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 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)
(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. :)
(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 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.
(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~:)
Created attachment 152903 [details] Patch
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 :-(
(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.
Created attachment 153724 [details] Patch
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.
Just FYI, Notification.permissionLevel() is going to be changed to Notification.permission() (bug 88919)
Created attachment 160621 [details] Patch
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 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 on attachment 160621 [details] Patch We started to add unit test for public API as well. Please add unit test.
Comment on attachment 160621 [details] Patch Set r- because of many comments.
Any update on this?
(In reply to comment #26) > Any update on this? I will update this soon. :)
(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. :)
(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.
(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.
(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.
Created attachment 162401 [details] Patch
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.
(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.
Created attachment 162411 [details] Patch
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 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 ?
Created attachment 162471 [details] Patch
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 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()).
(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.
Created attachment 162660 [details] Patch
Comment on attachment 162660 [details] Patch Clearing flags on attachment: 162660 Committed r127839: <http://trac.webkit.org/changeset/127839>
All reviewed patches have been landed. Closing bug.
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 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 ?
Reverted r127839 for reason: r127839 Committed r127848: <http://trac.webkit.org/changeset/127848>
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.