Summary: | [EFL][WK2] Implement web notification. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | SangYong Park <sy302.park> | ||||||||
Component: | WebKit EFL | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED WONTFIX | ||||||||||
Severity: | Normal | CC: | bunhere, cdumez, commit-queue, dbates, gyuyoung.kim, lucas.de.marchi, rakuco, sergio | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
SangYong Park
2014-04-10 01:08:58 PDT
Created attachment 229033 [details]
Patch
Comment on attachment 229033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229033&action=review API unit test should be added.(maybe in other bug.) > Source/WebKit2/UIProcess/API/efl/ewk_notification.cpp:55 > + m_ewkView = 0; s/0/nullptr > Source/WebKit2/UIProcess/API/efl/ewk_notification_permission_request_private.h:51 > + explicit EwkNotificationPermissionRequest(WKNotificationPermissionRequestRef, WKSecurityOriginRef); We do not need *explicit* keyword here. > Source/WebKit2/UIProcess/API/efl/ewk_notification_private.h:64 > + explicit EwkNotification(WKNotificationRef, EwkView*); We do not need *explicit* keyword here. Comment on attachment 229033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229033&action=review Good. I commented bit. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:83 > +#if ENABLE(NOTIFICATIONS) > + , m_notificationProvider(std::make_unique<NotificationProvider>(context)) > +#endif How about creating it as lazy as possible? And, should we add it in not ewk_view but ewk_context ? > Source/WebKit2/UIProcess/API/efl/ewk_notification.cpp:51 > +{ } We use different line for } > Source/WebKit2/UIProcess/efl/NotificationProvider.h:39 > +class NotificationProvider { NotificationProviderEfl? Comment on attachment 229033 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229033&action=review I think this patch needs to be reviewed for a long time because patch is too huge. :( > Source/WebKit/efl/ChangeLog:3 > + [EFL][WK2] Implement web notification. It would be good if you add [WK1] prefix as well. > Source/WebKit2/UIProcess/API/efl/ewk_notification.h:58 > + * save yourself from cpu cycles and use eina_stringshare_ref() I don't understand why this return type can be guaranteed by eina_stringshare. is the return char* protected by eina_stringshare_add() ? > Source/WebKit2/UIProcess/API/efl/ewk_notification.h:61 > +EAPI const char *ewk_notification_title_get(const Ewk_Notification *notification); Any unit test for this new APIs ? > Source/WebKit2/UIProcess/API/efl/ewk_notification.h:128 > + * @return the security origin object or @c NULL if there is not security origin. Do not use period at @return field. > Source/WebKit2/UIProcess/API/efl/ewk_notification_permission_request.h:58 > +EAPI Ewk_Security_Origin *ewk_notification_permission_request_security_origin_get(const Ewk_Notification_Permission_Request *permissionRequest); Any unit test for this new APIs ? permissionRequest -> permission_request > Source/WebKit2/UIProcess/API/efl/ewk_notification_permission_request.h:66 > +EAPI void ewk_notification_permission_request_allowed_set(Ewk_Notification_Permission_Request *permissionRequest, Eina_Bool allow); ditto. > Source/WebKit2/UIProcess/efl/NotificationProvider.h:26 > +#ifndef NotificationProvider_h EFL port has used *Efl* post fix. Please use NotificaitonProviderEfl_h for file name as well. Created attachment 229867 [details]
Patch
I will implement API and add unit test in additional patch. Comment on attachment 229867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229867&action=review > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:-451 > -NotificationClient* ChromeClientEfl::notificationPresenter() const Please remove this deprecated function in new bug. > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:-117 > -#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) ditto. > Tools/Scripts/webkitperl/FeatureList.pm:351 > + define => "ENABLE_NOTIFICATIONS", default => isEfl(), value => \$notificationsSupport }, I think we can enable this feature by default when implementation is finished. Created attachment 231491 [details]
Patch
Comment on attachment 229867 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=229867&action=review >> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:-451 >> -NotificationClient* ChromeClientEfl::notificationPresenter() const > > Please remove this deprecated function in new bug. done >> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.h:-117 >> -#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS) > > ditto. done >> Tools/Scripts/webkitperl/FeatureList.pm:351 >> + define => "ENABLE_NOTIFICATIONS", default => isEfl(), value => \$notificationsSupport }, > > I think we can enable this feature by default when implementation is finished. done Comment on attachment 231491 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=231491&action=review > Source/WebKit2/PlatformEfl.cmake:380 > + "${CMAKE_CURRENT_SOURCE_DIR}/UIProcess/API/efl/ewk_notification.h" It would be good if you open this new API files when you finish the implementation. > Source/WebKit2/UIProcess/API/efl/EWebKit2.h:48 > +#include "ewk_notification.h" ditto. > Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:78 > + , m_notificationProvider(std::make_unique<NotificationProviderEfl>(context)) Can I ask why you need to have ownership of m_notificationProvider ? If this is only for passing an pointer to WebCore side, I think we don't need to have this member variable and ownership. Comment on attachment 231491 [details] Patch Cleared review? from attachment 231491 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug or this bug again |