Bug 73544 – [EFL] Support for Web Notification.

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Kihong Kwon]

Add efl port implementation for the web notification.
This is already added to QT and Chromium port.

[Kihong Kwon]

Created attachment 117390 [details]
Add patch files for notification

[Ryuan Choi]

Comment on attachment 117390 [details]
Add patch files for notification

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

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:81
> +void NotificationPresenterClientEfl::makePermissionCache(char** domains, int size)

Can we pass Vector<String> instead of domains and size ?

> Source/WebKit/efl/ewk/ewk_private.h:238
> +void ewk_view_notification_request_permission(Evas_Object* ewkView, const char* domain);

permission_request?

> Source/WebKit/efl/ewk/ewk_view.h:2329
> +/**
> + * Make a cacheList for permitted domains
> + * This function is called when initializing a browser application.
> + *
> + * @param ewkView view to receive message from browser
> + * @param Ewk_Notification notification pointer for receive display event
> + *
> + */
> +EAPI void ewk_view_notification_add_permitted_domains(const Evas_Object* ewkView, char** domains, int size);

It doesn't looks EFL style.
we decide not to change o to ewkView in public header.

And IMO, We'd better to use Eina_List instead of char** and size.

And also, please check doxygen.

[Gyuyoung Kim]

Comment on attachment 117390 [details]
Add patch files for notification

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

> ChangeLog:6
> +        This is a implementation about web notification for efl

%s/a/an/g.

> ChangeLog:8
> +        * Source/cmake/OptionsEfl.cmake: elable ENABLE_NOTIFICATIONS feture

%s/elable/Enable/g

In addition, I think you need to make a test case for this feature.

> Source/WebKit/efl/ewk/ewk_private.h:234
> +WebCore::NotificationPresenter* ewk_view_notification_presenter_get(const Evas_Object *o);

Do not use abbreviation in internal function.

> Source/WebKit/efl/ewk/ewk_view.cpp:139
> +    WebCore::NotificationPresenter *notificationPresenter;

Move '*' to Data type.

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

Move ENABLE(NOTIFICATION) macro into function.

> Source/WebKit/efl/ewk/ewk_view.cpp:3938
> +WebCore::NotificationPresenter* ewk_view_notification_presenter_get(const Evas_Object* o)

Do not use abbreviation in internal function. Change o with ewkView.

> Source/WebKit/efl/ewk/ewk_view.cpp:3946
> +{

You should use ENABLE(NOTIFICATION) macro for implementation of public APIs.

> Source/WebKit/efl/ewk/ewk_view.cpp:3984
> +    // START TEMP

What does mean ?

> Source/WebKit/efl/ewk/ewk_view.cpp:3986
> +    // END TEMP

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:3996
> +void ewk_view_notification_clicked(const Evas_Object* ewkView, const Ewk_Notification* ewk_notification)

Do not use '_' in parameter variable.

> Source/WebKit/efl/ewk/ewk_view.cpp:4003
> +void ewk_view_notification_closed(const Evas_Object* ewkView, const Ewk_Notification* ewk_notification)

ditto.

> Source/WebKit/efl/ewk/ewk_view.cpp:4010
> +void ewk_view_notification_error(const Evas_Object* ewkView, const Ewk_Notification* ewk_notification)

ditto.

> Source/WebKit/efl/ewk/ewk_view.h:341
> +/* #if ENABLE(NOTIFICATIONS) */

Remove this comment.

> Source/WebKit/efl/ewk/ewk_view.h:351
> +/* #endif */

ditto.

> Source/WebKit/efl/ewk/ewk_view.h:2310
> +/* #if ENABLE(NOTIFICATIONS) */

Remove this comment.

> Source/WebKit/efl/ewk/ewk_view.h:2319
> +EAPI void ewk_view_notification_permission_callback(const Evas_Object* ewkView, Eina_Bool permitted, const char* domain);

Use abbreviation instead of full name

> Source/WebKit/efl/ewk/ewk_view.h:2338
> +EAPI void ewk_view_notification_displayed(const Evas_Object* ewkView, const Ewk_Notification*);

We should use abbreviation in public header files. 

See also EFL WebKit Coding Style document.
http://trac.webkit.org/wiki/EFLWebKitCodingStyle

> Source/WebKit/efl/ewk/ewk_view.h:2347
> +EAPI void ewk_view_notification_clicked(const Evas_Object* ewkView, const Ewk_Notification*);

ditto.

> Source/WebKit/efl/ewk/ewk_view.h:2356
> +EAPI void ewk_view_notification_closed(const Evas_Object* ewkView, const Ewk_Notification*);

ditto.

> Source/WebKit/efl/ewk/ewk_view.h:2366
> +/* #endif */

Remove this comment.

> Tools/ChangeLog:6
> +        This is a implementation about web notification for efl

%s/a/an/g

[Kihong Kwon]

(In reply to comment #2)
> (From update of attachment 117390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117390&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:81
> > +void NotificationPresenterClientEfl::makePermissionCache(char** domains, int size)
> 
> Can we pass Vector<String> instead of domains and size ?
> 

If I change from char** to Vector<String>, I have to make a Vector<String> for domain lists before call this method and change to HashSet<String> from Vector<String>in this method.

I think this is not good for efficiency.

> > Source/WebKit/efl/ewk/ewk_private.h:238
> > +void ewk_view_notification_request_permission(Evas_Object* ewkView, const char* domain);
> 
> permission_request?
>

I don't think so, because this function is a bridge to WebCore::NotificationPresenter::requestPermission()
So, I think request_permission is better for consistency
 
> > Source/WebKit/efl/ewk/ewk_view.h:2329
> > +/**
> > + * Make a cacheList for permitted domains
> > + * This function is called when initializing a browser application.
> > + *
> > + * @param ewkView view to receive message from browser
> > + * @param Ewk_Notification notification pointer for receive display event
> > + *
> > + */
> > +EAPI void ewk_view_notification_add_permitted_domains(const Evas_Object* ewkView, char** domains, int size);
> 
> It doesn't looks EFL style.
> we decide not to change o to ewkView in public header.

OK.

> 
> And IMO, We'd better to use Eina_List instead of char** and size.
> 

Is there any benefit? or have I to use Eina_List for char**?

> And also, please check doxygen.

OK.

[Kihong Kwon]

Created attachment 118556 [details]
Add fixed patch for comments

[Kihong Kwon]

Fix all of your comment

(In reply to comment #3)
> (From update of attachment 117390 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117390&action=review
> 
> > ChangeLog:6
> > +        This is a implementation about web notification for efl
> 
> %s/a/an/g.
> 
> > ChangeLog:8
> > +        * Source/cmake/OptionsEfl.cmake: elable ENABLE_NOTIFICATIONS feture
> 
> %s/elable/Enable/g
> 
> In addition, I think you need to make a test case for this feature.
> 
> > Source/WebKit/efl/ewk/ewk_private.h:234
> > +WebCore::NotificationPresenter* ewk_view_notification_presenter_get(const Evas_Object *o);
> 
> Do not use abbreviation in internal function.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:139
> > +    WebCore::NotificationPresenter *notificationPresenter;
> 
> Move '*' to Data type.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3937
> > +#if ENABLE(NOTIFICATIONS)
> 
> Move ENABLE(NOTIFICATION) macro into function.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3938
> > +WebCore::NotificationPresenter* ewk_view_notification_presenter_get(const Evas_Object* o)
> 
> Do not use abbreviation in internal function. Change o with ewkView.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3946
> > +{
> 
> You should use ENABLE(NOTIFICATION) macro for implementation of public APIs.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3984
> > +    // START TEMP
> 
> What does mean ?
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3986
> > +    // END TEMP
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:3996
> > +void ewk_view_notification_clicked(const Evas_Object* ewkView, const Ewk_Notification* ewk_notification)
> 
> Do not use '_' in parameter variable.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4003
> > +void ewk_view_notification_closed(const Evas_Object* ewkView, const Ewk_Notification* ewk_notification)
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:4010
> > +void ewk_view_notification_error(const Evas_Object* ewkView, const Ewk_Notification* ewk_notification)
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:341
> > +/* #if ENABLE(NOTIFICATIONS) */
> 
> Remove this comment.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:351
> > +/* #endif */
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2310
> > +/* #if ENABLE(NOTIFICATIONS) */
> 
> Remove this comment.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2319
> > +EAPI void ewk_view_notification_permission_callback(const Evas_Object* ewkView, Eina_Bool permitted, const char* domain);
> 
> Use abbreviation instead of full name
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2338
> > +EAPI void ewk_view_notification_displayed(const Evas_Object* ewkView, const Ewk_Notification*);
> 
> We should use abbreviation in public header files. 
> 
> See also EFL WebKit Coding Style document.
> http://trac.webkit.org/wiki/EFLWebKitCodingStyle
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2347
> > +EAPI void ewk_view_notification_clicked(const Evas_Object* ewkView, const Ewk_Notification*);
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2356
> > +EAPI void ewk_view_notification_closed(const Evas_Object* ewkView, const Ewk_Notification*);
> 
> ditto.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:2366
> > +/* #endif */
> 
> Remove this comment.
> 
> > Tools/ChangeLog:6
> > +        This is a implementation about web notification for efl
> 
> %s/a/an/g

[Gyuyoung Kim]

Comment on attachment 118556 [details]
Add fixed patch for comments

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

> Source/WebKit/efl/ChangeLog:8
> +        This is a implementation about web notification for efl

s/a/an/g

> Source/WebKit/efl/ewk/ewk_view.h:2298
> +EAPI void ewk_view_notification_permission_callback(const Evas_Object *o, Eina_Bool permitted, const char* domain);

I don't like to implement new independent feature to ewk_view. I think it is better to add new files (e.g ewk_notification.cpp | h) for this feature. I think you can move ewk_view_notificaiton_xxx functions to ewk_notification.cpp by using Ewk_Notification struct.

[Kihong Kwon]

Created attachment 118735 [details]
Separate functions for the notification from ewk_view to ewk_notification

[WebKit Review Bot]

Attachment 118735 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebKit/efl/CMakeLists..." exit_code: 1

Source/WebKit/efl/ewk/ewk_notification.h:23:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/efl/ewk/ewk_notification.h:24:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/efl/ewk/ewk_notification.h:25:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/efl/ewk/ewk_notification.h:26:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/efl/ewk/ewk_notification.h:31:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:48:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit/efl/ewk/ewk_notification.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 7 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kihong Kwon]

Created attachment 118738 [details]
Separate functions for the notification from ewk_view to ewk_notification

[Gyuyoung Kim]

Comment on attachment 118738 [details]
Separate functions for the notification from ewk_view to ewk_notification

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

> Source/WebKit/efl/ewk/ewk_notification.cpp:81
> +void ewk_notification_show(Evas_Object* ewkView, WebCore::Notification* notification)

If ewk_notificaiton.cpp only uses ewkView object for sending signal, it looks there are no problems. However, if we need to expand new attribute in future, I think we need to make smart data for ewk_notification.cpp. Then, we can bind it with ewkView object as Ewk_Frame_Smart_Data.

> Source/WebKit/efl/ewk/ewk_notification.h:43
> + * Callback call after user approve display a notification.

approve -> approves ?

> Source/WebKit/efl/ewk/ewk_notification.h:64
> + * Make a ondisplay callback call for JavaScript after notification is displayed

%s/a/an/g

> Source/WebKit/efl/ewk/ewk_notification.h:73
> + * Make a onclick callback call for JavaScript after notification is clicked

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:82
> + * Make a onclose callback call for JavaScript after notification is closed

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:99
> +WebCore::NotificationPresenter* ewk_notification_presenter_get(const Evas_Object* ewkView);

This is internal function, right? We should not open internal function to outer.

> Source/WebKit/efl/ewk/ewk_notification.h:100
> +void ewk_notification_show(Evas_Object* ewkView, WebCore::Notification* notification);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:101
> +void ewk_notification_html_show(Evas_Object* ewkView, WebCore::Notification* notification);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:102
> +void ewk_notification_cancel(Evas_Object* ewkView, WebCore::Notification* notification);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:103
> +void ewk_notification_request_permission(Evas_Object* ewkView, const char* domain);

ditto.

[Gyuyoung Kim]

CC'ing Kubo, Grzegorz.

[Kihong Kwon]

Created attachment 119165 [details]
It's fixed about commented

[Grzegorz Czajkowski]

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

> ChangeLog:3
> +        [EFL] Add Web Notification feature for Efl port

I'd rather mention that API is added in this patch and please add dot:
[EFL] Add API for Web Notification feature.

> ChangeLog:6
> +        This is an implementation about web notification for efl

Missing dot.

> Source/WebKit/efl/ChangeLog:3
> +        [EFL] Add Web Notification feature for Efl port

Please change subject as I mentioned above.

> Source/WebKit/efl/ChangeLog:8
> +        This is an implementation about web notification for efl

Missing dot.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:49
> +

In my opinion this an empty line is not necessary.

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

What do you say about adding this without assign it to the String variable?
m_cachedPermissions.add(String(domains[i]));
If you agree with this suggestion, please remove unnecessary {} in for.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:94
> +

Unnecessary an empty line.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:95
> +    m_cachedPermissions.add(permittedDomain);

m_cachedPermissions.add(String(domain));

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:53
> +    Evas_Object* m_view;

I would rather change an order here. Please define members of class first then methods.

> Source/WebKit/efl/ewk/ewk_notification.cpp:101
> +

Unnecessary an empty line.

> Source/WebKit/efl/ewk/ewk_notification.cpp:108
> +

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:19
> +

Please add doxygen section here:
/**
 * @file ....
 * @brief ...
 *
 * Detailed description.
 */

> Source/WebKit/efl/ewk/ewk_notification.h:32
> +typedef struct _Ewk_Notification Ewk_Notification;

Generally WebKit-Efl documents typedefs too.

> Source/WebKit/efl/ewk/ewk_notification.h:35
> +    void* notification;

Please describe these struct members.

> Source/WebKit/efl/ewk/ewk_notification.h:43
> + * Call a callback fucntion which is resisterd, after receiving a user approval.

Call -> Calls
resisterd -> registered

> Source/WebKit/efl/ewk/ewk_notification.h:45
> + * @param ewkView view to receive message from browser

@param ewkView view to call registered function

> Source/WebKit/efl/ewk/ewk_notification.h:46
> + * @param permitted whether permit a notificaion or not by user

* @param permitted @c EINA_TRUE to allow (what? notificaion or callbakc), @c EINA_FALSE to ...

> Source/WebKit/efl/ewk/ewk_notification.h:53
> + * Make a cacheList for permitted domains

Make -> Makes
Please do not use variable names here.
Missing dot.
Please add en empty line here to signal detailed description.

> Source/WebKit/efl/ewk/ewk_notification.h:54
> + * This function is called when initializing a browser application.

This comment confuses developer. As I understand your intention  this API should be called if application wants to has some benefits of it.
And please do not user "browser" in comments. Many applications use WebKit not only browser.

> Source/WebKit/efl/ewk/ewk_notification.h:56
> + * @param ewkView view to receive message from browser

to receive message from browser -> to add permitted domains

> Source/WebKit/efl/ewk/ewk_notification.h:57
> + * @param domains domain list which is added to permission cache list

* @param domains domain list which is added - please describe what benefits are from adding these (notification?)

> Source/WebKit/efl/ewk/ewk_notification.h:64
> + * Make an ondisplay callback call for JavaScript after notification is displayed

Calls ondisplay callback ... ?

> Source/WebKit/efl/ewk/ewk_notification.h:66
> + * @param ewkView view to receive message from browser

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:72
> +/**

This section should be changed regarding to the previous comments.

> Source/WebKit/efl/ewk/ewk_notification.h:81
> +/**

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:90
> +/**

Ditto.

> Tools/ChangeLog:3
> +        [EFL] Add Web Notification feature for Efl port

Please change a subject.

> Tools/ChangeLog:6
> +        This is an implementation about web notification for efl

Missing dot.

[Kihong Kwon]

> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:53
> > +    Evas_Object* m_view;

>I would rather change an order here. Please define members of class first then methods.

Could you show me a example please?
I can't find private member function from WebCoreSupport but 
I think members are first then methods in the WebCore.
Please see a DOMWindow.h.

How do you think about this?


(In reply to comment #14)
> View in context: https://bugs.webkit.org/attachment.cgi?id=119165&action=review
> 
> > ChangeLog:3
> > +        [EFL] Add Web Notification feature for Efl port
> 
> I'd rather mention that API is added in this patch and please add dot:
> [EFL] Add API for Web Notification feature.
> 
> > ChangeLog:6
> > +        This is an implementation about web notification for efl
> 
> Missing dot.
> 
> > Source/WebKit/efl/ChangeLog:3
> > +        [EFL] Add Web Notification feature for Efl port
> 
> Please change subject as I mentioned above.
> 
> > Source/WebKit/efl/ChangeLog:8
> > +        This is an implementation about web notification for efl
> 
> Missing dot.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:49
> > +
> 
> In my opinion this an empty line is not necessary.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:87
> > +        m_cachedPermissions.add(domain);
> 
> What do you say about adding this without assign it to the String variable?
> m_cachedPermissions.add(String(domains[i]));
> If you agree with this suggestion, please remove unnecessary {} in for.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:94
> > +
> 
> Unnecessary an empty line.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:95
> > +    m_cachedPermissions.add(permittedDomain);
> 
> m_cachedPermissions.add(String(domain));
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:53
> > +    Evas_Object* m_view;
> 
> I would rather change an order here. Please define members of class first then methods.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:101
> > +
> 
> Unnecessary an empty line.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:108
> > +
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:19
> > +
> 
> Please add doxygen section here:
> /**
>  * @file ....
>  * @brief ...
>  *
>  * Detailed description.
>  */
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:32
> > +typedef struct _Ewk_Notification Ewk_Notification;
> 
> Generally WebKit-Efl documents typedefs too.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:35
> > +    void* notification;
> 
> Please describe these struct members.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:43
> > + * Call a callback fucntion which is resisterd, after receiving a user approval.
> 
> Call -> Calls
> resisterd -> registered
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:45
> > + * @param ewkView view to receive message from browser
> 
> @param ewkView view to call registered function
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:46
> > + * @param permitted whether permit a notificaion or not by user
> 
> * @param permitted @c EINA_TRUE to allow (what? notificaion or callbakc), @c EINA_FALSE to ...
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:53
> > + * Make a cacheList for permitted domains
> 
> Make -> Makes
> Please do not use variable names here.
> Missing dot.
> Please add en empty line here to signal detailed description.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:54
> > + * This function is called when initializing a browser application.
> 
> This comment confuses developer. As I understand your intention  this API should be called if application wants to has some benefits of it.
> And please do not user "browser" in comments. Many applications use WebKit not only browser.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:56
> > + * @param ewkView view to receive message from browser
> 
> to receive message from browser -> to add permitted domains
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:57
> > + * @param domains domain list which is added to permission cache list
> 
> * @param domains domain list which is added - please describe what benefits are from adding these (notification?)
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:64
> > + * Make an ondisplay callback call for JavaScript after notification is displayed
> 
> Calls ondisplay callback ... ?
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:66
> > + * @param ewkView view to receive message from browser
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:72
> > +/**
> 
> This section should be changed regarding to the previous comments.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:81
> > +/**
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:90
> > +/**
> 
> Ditto.
> 
> > Tools/ChangeLog:3
> > +        [EFL] Add Web Notification feature for Efl port
> 
> Please change a subject.
> 
> > Tools/ChangeLog:6
> > +        This is an implementation about web notification for efl
> 
> Missing dot.

[Grzegorz Czajkowski]

(In reply to comment #15)
> > > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:53
> > > +    Evas_Object* m_view;
> 
> >I would rather change an order here. Please define members of class first then methods.
> 
> Could you show me a example please?
> I can't find private member function from WebCoreSupport but 
> I think members are first then methods in the WebCore.
> Please see a DOMWindow.h.
> 
> How do you think about this?
> 

In private section you've defined sendEvent(...) method first and then variables.
I'd rather reverse them.

[Kihong Kwon]

Created attachment 119191 [details]
Fixed for comment #14

[Grzegorz Czajkowski]

Thanks for your changes Kihong.
I did a review again and some fixes are still needed especially in doxygen comments. Please see other ewk API documentation and try to adjust your comments.
Please consider changing API name. I suggest you to notify this API by WebKit-EFL list.


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

> ChangeLog:6
> +        This is an implementation about web notification for efl.

I would rather remove this general description from here and add what you really change. For example:
Enables Web Notification feature for WebKit-Efl.

> ChangeLog:8
> +        * Source/cmake/OptionsEfl.cmake: Enable ENABLE_NOTIFICATIONS feture

1) If you agree with my previous comment this an additional description isn't necessary.
2) I don't see your modifications in Source/cmake/OptionsEfl.cmake but in Source/WebKit/efl/CMakeListsEfl.txt. Please verify ChangeLog.

> Source/WebKit/efl/ChangeLog:8
> +        This is an implementation about web notification for efl.

This description needs changes, what do you say about this?
Implements Web Notification feature and exposes API for WebKit-EFl.
This an implementation is based on webkitNotificaion property in window.navigator.

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

This value is also returned in previous condition. Please consider to merger them into one, for example

if (!m_cachedPermissions.size()
    || m_cachedPermissions.find(context->securityOrigin()->toString()) == m_cachedPermissions.end())
    return PermissionDenied;

> Source/WebKit/efl/ewk/ewk_notification.cpp:62
> +    if (!permit)

I would rather reverse logic and call callback if permit is true.

> Source/WebKit/efl/ewk/ewk_notification.h:22
> + * @brief APIs for the web notification of WebKit-EFL

Please describe all signals which are emitted by this API, for example notification,contents,show, notification,contents,htmlshow etc.

> Source/WebKit/efl/ewk/ewk_notification.h:39
> + */

/// Creates a type name for @c _Ewk_Notification.

> Source/WebKit/efl/ewk/ewk_notification.h:41
> +

/// Contains Web Notification data.

> Source/WebKit/efl/ewk/ewk_notification.h:43
> +    void* notification;

Should user have access to this member? In my opinion it should be moved to an implementation file.

> Source/WebKit/efl/ewk/ewk_notification.h:44
> +    char* iconURL;

Please describe this variable because it's accessible from API.

> Source/WebKit/efl/ewk/ewk_notification.h:45
> +    char* title;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:46
> +    char* body;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:47
> +    char* url;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:51
> + * Calls a callback fucntion which is registerd, after receiving a user approval.

It would be nice to describe what the function does for caller.
Please describe what benefits are when this callback is called.

> Source/WebKit/efl/ewk/ewk_notification.h:53
> + * @param ewkView view to call registered function

This comment rather notify caller that he should register function. But it registered by WebCore. Am I right?

> Source/WebKit/efl/ewk/ewk_notification.h:54
> + * @param permitted @c EINA_TRUE to allow to show notificaion, @c EINA_FALSE to not allow to show notificaiton.

1) notificaion -> notification
2) notificaiton -> notification

> Source/WebKit/efl/ewk/ewk_notification.h:56
> + *

Unnecessary an empty line.

> Source/WebKit/efl/ewk/ewk_notification.h:58
> +EAPI void ewk_notification_permission_callback(const Evas_Object *o, Eina_Bool permitted, const char* domain);

This API name should be renamed, please use verb in API name, for example:
wk_notification_permission_callback -> wk_notification_permission_callback_add ?

> Source/WebKit/efl/ewk/ewk_notification.h:61
> + * Makes a Cache for permitted domains

Missing dot.

> Source/WebKit/efl/ewk/ewk_notification.h:63
> + * This function can be called at initializing time if a application have permmited domains.

permmited -> permitted

> Source/WebKit/efl/ewk/ewk_notification.h:71
> +EAPI void ewk_notification_add_permitted_domains(const Evas_Object *o, char** domains, int size);

ewk_notification_add_permitted_domains -> ewk_notification_permitted_domains_add ?

> Tools/ChangeLog:6
> +        This is an implementation about web notification for efl.

I would rather remove this general description from here and add what you really change

[Kihong Kwon]

Created attachment 119559 [details]
Fixed for comment #18

Thank you for your comment Grzegorz~:)

[Gyuyoung Kim]

Comment on attachment 119559 [details]
Fixed for comment #18

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

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:67
> +        ewk_notification_request_permission(m_view, context->securityOrigin()->toString().utf8().data());

In GTK port, each file for independent feature has private data structure. The private structure has reference of webView.
webview interacts with independent feature via the private structure.

For example,

In ChromeClientGtk.cpp,

643 void ChromeClient::dispatchViewportPropertiesDidChange(const ViewportArguments& arguments) const
644 {
645     // Recompute the viewport attributes making it valid.
646     webkitViewportAttributesRecompute(webkit_web_view_get_viewport_attributes(m_webView));
647 }

In webkitviewportattributes.cpp

519 void webkitViewportAttributesRecompute(WebKitViewportAttributes* viewportAttributes)
520 {
521     WebKitViewportAttributesPrivate* priv = viewportAttributes->priv;
522     WebKitWebView* webView = priv->webView;
523 
524     IntRect windowRect(webView->priv->corePage->chrome()->windowRect());
525     priv->deviceWidth = windowRect.width();
526     priv->deviceHeight = windowRect.height();
527     



In webkitviewportattributesprivate.h,

 30 struct _WebKitViewportAttributesPrivate {
 31     WebKitWebView* webView;
 32     int deviceWidth;
 33     int deviceHeight;
 34     int availableWidth;
 35     int availableHeight;
 36     int desktopWidth;
 37     int deviceDPI;
 38     
 39     int width;
 40     int height;
 41     float initialScaleFactor;
 42     float minimumScaleFactor;
 43     float maximumScaleFactor;
 44     float devicePixelRatio;
 45     gboolean userScalable;
 46     gboolean isValid;
 47 };

In webkitviewportattributes.cpp, it uses webView instance via private structure. I think EFL port also needs to have similar structure.

[Kihong Kwon]

Created attachment 120653 [details]
Use Ewk_Notification structure for the ewk_notificaiton.h/cpp

[Kihong Kwon]

Created attachment 121052 [details]
Add management routines for notification objects

[Kihong Kwon]

Created attachment 121392 [details]
Fix errors for the build.

[Kihong Kwon]

Created attachment 121398 [details]
Add destroy notification objects for the ewk.

[Gyuyoung Kim]

Comment on attachment 121398 [details]
Add destroy notification objects for the ewk.

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

> Source/WebKit/efl/ewk/ewk_notification.cpp:124
> +    Ewk_Notification* notification = static_cast<Ewk_Notification*>(malloc(sizeof(Ewk_Notification)));

Please use new instead of malloc.

See : https://bugs.webkit.org/show_bug.cgi?id=73544

> Source/WebKit/efl/ewk/ewk_notification.cpp:125
> +    if (!notification)

new never return 0. Remove this.

> Source/WebKit/efl/ewk/ewk_notification.cpp:138
> +    free(ewkNotification);

Use delete instead of free.

> Source/WebKit/efl/ewk/ewk_notification.cpp:144
> +    Ewk_Notification_Info* notification_info = static_cast<Ewk_Notification_Info*>(malloc(sizeof(Ewk_Notification_Info)));

ditto.

> Source/WebKit/efl/ewk/ewk_notification.cpp:146
> +        return 0;

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:2
> +    Copyright (C) 2011 Samsung Electronics

Change year.

> Source/WebKit/efl/ewk/ewk_notification.h:35
> + *  - "notification,show", @c Ewk_Notification_Info: requested contents to display the notification.

This signal is emitted by view. So, this comment needs to be moved to ewk_view.h

> Source/WebKit/efl/ewk/ewk_private.h:234
> +#if ENABLE(NOTIFICATIONS)

I think we can move this internal functions to ewk_notificaiton_private.h. Bug 75592 also try to add similar private header file. If the Bug's patch is landed, this patch also can has similar file.

> Source/WebKit/efl/ewk/ewk_view.cpp:3916
> +WebCore::NotificationPresenter* ewk_view_notification_presenter_get(const Evas_Object* ewkView)

Can we move this function to ewk_notification?

We can get page object as below,
WebCore::Page* page = EWKPrivate::corePage(priv->ewkView);

[Kihong Kwon]

Created attachment 121414 [details]
Fixed for the all of comments(#25)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 121414 [details]
Fixed for the all of comments(#25)

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

This still needs *a lot* of love before getting into trunk.

Starting with the basics: there are more English mistakes than usual, the documentation is not helpful at all for some random API user who comes across this part of ewk for the first time, there are unused #includes in many files, EWebKit.h will break if ewk_notification.h is not installed, there are many typos and useless statements (eg. void foo(MyObject* ptr) { /* do something */ ptr = 0; }). If possible, I highly recommend doing some internal reviews to spot all these mistakes before sending the bug upstream, so at least some back-and-forth is avoided and reviewers can concentrate on the real issues.

Now for the real issues (many parts have been commented inline): this is over-engineered and the architecture is not very good.
 * Let's start with the distinction between Ewk_Notification and Ewk_Notification_Info:
   - The names chose are really unhelpful. According to the spec (and, to some extent, the API in WebCore), a Notification is a single notificaton shown to the user. Withing ewk, a Notification actually corresponds to an Ewk_Notification_Info, and Ewk_Notification is a poor man's controller.
   - Ewk_Notification does not really need to exist. Of its 3 attributes, ewk_view is not needed (just store the view in NotificationPresenterEfl's constructor), ewk_notification_info_list can be stored in the presenter itself (using a WTF data structure) and notification_presenter does not need to exist if you just point to it from an Ewk_Notification_Info. You can then rename Ewk_Notification_Info to Ewk_Notification and get rid of some of the unhelpful abstraction.
 * The queueing of callback calls seems flawed (a single callback is kept at a time).
 * "ewk_notification_{displayed,clicked,closed,error}" are bad names. The last one is not consistent with the rest, and a reader of the API cannot really tell what they do without looking at the code or the documentation (which is also very minimal). "ewk_notification_notify_clicked" etc (or something along those lines) is more helpful (I don't like ewk code calling code in WebCoreSupport, but it seems we can't avoid it this time).
 * The granularity of the object called in "notification,show" looks wrong.. A security origin is usually associated with a frame, not a view (a view can have many frames); thus emitting "notification,show" for a view loses some information in the process.
 * ewk_notification_permitted_domains_add looks like an ewk_frame function for the reason I mentioned above (in the worst case, an ewk_frame should be passed to this function).
Some of the analysis may be biased because I was looking at the Qt port's code while reviewing this patch, so other ways to approach these issues are welcome.

Last but not least: I don't know how much this patch has been tested in production code, but from the current looks of it, I would _really_ like the notification-related layout tests to pass (which means some small DRT code probably needs to be written) before this is landed. I'm not 100% confident the current code does everything the spec says (for example, I don't see anything related to replaceId), so at least having the tests passing makes me not need to trust my gut feelings that much.

> Source/WebKit/efl/CMakeListsEfl.txt:98
> +IF (ENABLE_NOTIFICATIONS)
> +    LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> +        "${WEBCORE_DIR}/notifications"
> +    )
> +    LIST(APPEND WebKit_SOURCES
> +        efl/ewk/ewk_notification.cpp
> +    )
> +ENDIF ()

Why has the block been moved?

> Source/WebKit/efl/CMakeListsEfl.txt:259
> +    LIST(APPEND EWebKit_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_notification_private.h)

A private header must not be installed.

> Source/WebKit/efl/ChangeLog:57
> +        * ewk/ewk_private.h:
> +        * ewk/ewk_view.cpp:
> +        (_ewk_view_priv_new):
> +        * ewk/ewk_view.h:

At least the files which have been changed could have some description of what has been changed.

> Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:425
>  #if ENABLE(NOTIFICATIONS)
>  NotificationPresenter* ChromeClientEfl::notificationPresenter() const
>  {
> -    notImplemented();
> -    return 0;
> +    return ewk_notification_presenter_get(m_view);
>  }

This has been removed from the parent class in r101307, so this method and ewk_notification_presenter_get can be removed altogether.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:50
> +    if (notification->isHTML()) {
> +        Ewk_Notification_Info* ewkNotificationInfo = ewk_notification_info_create(m_ewkNotification, notification);
> +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,htmlshow", ewkNotificationInfo);
> +    } else {
> +        Ewk_Notification_Info* ewkNotificationInfo = ewk_notification_info_create(m_ewkNotification, notification);
> +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,show", ewkNotificationInfo);
> +    }

Why not have a boolean in the object which tells whether it is an HTML notification or not and then just emit a single type of signal?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-54
> -    notImplemented();

Why? This is still not implemented.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:74
> +    if (m_cachedPermissions.find(context->securityOrigin()->toString()) == m_cachedPermissions.end()) {

I don't really see why the cached permissions are being searched (mostly because other ports don't do that).

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:75
> +        m_callback = callback;

This method can be called many times and for multiple security origins. How do you handle that with a single variable?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:76
> +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,requestPermission" \

Unneeded "\" at the end of the line.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> +            , (void*)context->securityOrigin()->toString().utf8().data());

The C-style cast is not needed, and we do not use C-style casts.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:27
> +#include <Ecore_Evas.h>

Why?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:43
> +    virtual void cancelRequestsForPermission(ScriptExecutionContext*) { }

Doesn't this need to be implemented?

> Source/WebKit/efl/ewk/EWebKit.h:40
> +#include "ewk_notification.h"

If ewk_notification.h is not installed, this will break the build for users.

> Source/WebKit/efl/ewk/ewk_notification.cpp:26
> +#include "ewk_logging.h"

Unused.

> Source/WebKit/efl/ewk/ewk_notification.cpp:37
> +    WebCore::NotificationPresenterClientEfl* notification_presenter;
> +    Evas_Object* ewk_view;
> +    Eina_List* ewk_notification_info_list;

Wrong naming style.

> Source/WebKit/efl/ewk/ewk_notification.cpp:41
> +    Ewk_Notification* ewk_notification;

Ditto.

> Source/WebKit/efl/ewk/ewk_notification.cpp:42
> +    void* notification;

Why void?

> Source/WebKit/efl/ewk/ewk_notification.cpp:47
> +    unsigned __ref;

This is not used anywhere and the naming is weird.

> Source/WebKit/efl/ewk/ewk_notification.cpp:126
> +Ewk_Notification* ewk_notification_create(WebCore::NotificationPresenter* notificationPresenter, Evas_Object* ewkView)
> +{
> +    Ewk_Notification* notification = new Ewk_Notification;
> +    notification->notification_presenter = static_cast<WebCore::NotificationPresenterClientEfl*>(notificationPresenter);

Why all this casting if you are already passed a NotificationPresenterClientEfl? Just change the argument type in the function.

> Source/WebKit/efl/ewk/ewk_notification.cpp:133
> +void ewk_notification_destroy(Ewk_Notification* ewkNotification)

What's the point of having this function if it just calls destroy_all? This all could be done directly in the presenter's destructor.

> Source/WebKit/efl/ewk/ewk_notification.cpp:137
> +    ewkNotification = 0;

This has absolutely no effect.

> Source/WebKit/efl/ewk/ewk_notification.cpp:150
> +    notification_info->iconURL = strdup(static_cast<WebCore::Notification*>(notification)->contents().icon.string().utf8().data());
> +    notification_info->title = strdup(static_cast<WebCore::Notification*>(notification)->contents().title.utf8().data());
> +    notification_info->body = strdup(static_cast<WebCore::Notification*>(notification)->contents().body.utf8().data());
> +    notification_info->htmlURL = strdup(static_cast<WebCore::Notification*>(notification)->url().string().utf8().data());

Why not use stringshares here?

> Source/WebKit/efl/ewk/ewk_notification.cpp:167
> +    if (!ewkNotificationInfo->iconURL)
> +        free(ewkNotificationInfo->iconURL);
> +    if (!ewkNotificationInfo->iconURL)
> +        free(ewkNotificationInfo->title);
> +    if (!ewkNotificationInfo->iconURL)
> +        free(ewkNotificationInfo->body);
> +    if (!ewkNotificationInfo->iconURL)
> +        free(ewkNotificationInfo->htmlURL);

The checks here are calling free() only if the variables are already 0. Plus, the checks are not needed at all. free(0) is a NOP.

> Source/WebKit/efl/ewk/ewk_notification.cpp:172
> +    ewkNotificationInfo = 0;

This line has no effect.

> Source/WebKit/efl/ewk/ewk_notification.cpp:175
> +void ewk_notificaiton_destroy_all(Ewk_Notification* ewkNotification)

Typo: "notificaiton"

> Source/WebKit/efl/ewk/ewk_notification.h:23
> +#include <Evas.h>

What is the Evas.h include used for? EAPI, for one, is in Eina.

> Source/WebKit/efl/ewk/ewk_notification.h:34
> + * The following signals (see evas_object_smart_callback_add()) are emitted:

If there is no signal being emitted, there is no point in having this line.

Plus, some reference to the HTML5 spec is useful so people know what a web notification is.

> Source/WebKit/efl/ewk/ewk_notification.h:44
> + * Get iconURL for the notification from @c Ewk_Notification_Info.

"iconURL" is very implementation specific for a documentation entry. Please explain what it means and separate "icon" and "URL" (and in the "@return" line you use "Icon URL", with spaces, different order and different casing.

> Source/WebKit/efl/ewk/ewk_notification.h:48
> + * @return chacter pointer to the Icon URL of notification.

Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.

> Source/WebKit/efl/ewk/ewk_notification.h:49
> + *

This empty line can be removed.

> Source/WebKit/efl/ewk/ewk_notification.h:58
> + * @return chacter pointer to the title of notification.

Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.

> Source/WebKit/efl/ewk/ewk_notification.h:59
> + *

This empty line can be removed.

> Source/WebKit/efl/ewk/ewk_notification.h:68
> + * @return chacter pointer to the body contents of notification.

Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.

> Source/WebKit/efl/ewk/ewk_notification.h:69
> + *

This empty line can be removed.

> Source/WebKit/efl/ewk/ewk_notification.h:74
> + * Get html contents URL for the notification from @c Ewk_Notification_Info.

This is not mentioned in the w3c spec, so please explain what "html contents URL" means.

> Source/WebKit/efl/ewk/ewk_notification.h:78
> + * @return chacter pointer to the html contents url of notification.

Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.

> Source/WebKit/efl/ewk/ewk_notification.h:79
> + *

This empty line can be removed.

> Source/WebKit/efl/ewk/ewk_notification.h:84
> + * Calls a callback fucntion which is registerd, after receiving a user approval.

Typo: "fucntion" and "registerd". And it should be "user's approval".

> Source/WebKit/efl/ewk/ewk_notification.h:90
> +EAPI void ewk_notification_permission_callback_call(const Ewk_Notification_Info* eni, Eina_Bool permitted, const char* domain);

I don't see why this should be called by users. The callback must be called automatically when the user has responded to the permission request.

> Source/WebKit/efl/ewk/ewk_notification.h:93
> + * Makes a Cache for permitted domains.

Why is "Cache" capitalized? What should it mean for the reader?

> Source/WebKit/efl/ewk/ewk_notification.h:95
> + * This function can be called at initializing time if a application have permited domains.

Typo: "permited". "initializing" should be "initialization".

> Source/WebKit/efl/ewk/ewk_notification.h:96
> + * WebCore checks a permission from the cache only when checkPermission() is called from javascript.

API users are not expected to know what "WebCore" means.

> Source/WebKit/efl/ewk/ewk_notification.h:102
> +EAPI void ewk_notification_permitted_domains_add(const Ewk_Notification_Info* eni, char** domains, int size);

The description above contradicts the code's use: this is currently the only way to add permission for a given security origin and thus cannot be called "at initializing time".

> Source/WebKit/efl/ewk/ewk_notification_private.h:43
> +void ewk_notificaiton_destroy_all(Ewk_Notification* ewkNotification);

Typo: "notificaiton"

> Source/WebKit/efl/ewk/ewk_private.h:-230
> -

Unrelated change.

> Source/WebKit/efl/ewk/ewk_view.cpp:71
> +#include "NotificationController.h"

Is this include really needed?

> Tools/ChangeLog:9
> +        And add "WebCore/notifications" source path to build DumpRenderTree.

No need to say this here if you repeat it below.

[Kihong Kwon]

Firstly Thank you for your comment. Kubo.

(In reply to comment #27)
> (From update of attachment 121414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121414&action=review
> 
> This still needs *a lot* of love before getting into trunk.
> 
> Starting with the basics: there are more English mistakes than usual, the documentation is not helpful at all for some random API user who comes across this part of ewk for the first time, there are unused #includes in many files, EWebKit.h will break if ewk_notification.h is not installed, there are many typos and useless statements (eg. void foo(MyObject* ptr) { /* do something */ ptr = 0; }). If possible, I highly recommend doing some internal reviews to spot all these mistakes before sending the bug upstream, so at least some back-and-forth is avoided and reviewers can concentrate on the real issues.
> 
> Now for the real issues (many parts have been commented inline): this is over-engineered and the architecture is not very good.
>  * Let's start with the distinction between Ewk_Notification and Ewk_Notification_Info:
>    - The names chose are really unhelpful. According to the spec (and, to some extent, the API in WebCore), a Notification is a single notificaton shown to the user. Withing ewk, a Notification actually corresponds to an Ewk_Notification_Info, and Ewk_Notification is a poor man's controller.
>    - Ewk_Notification does not really need to exist. Of its 3 attributes, ewk_view is not needed (just store the view in NotificationPresenterEfl's constructor), ewk_notification_info_list can be stored in the presenter itself (using a WTF data structure) and notification_presenter does not need to exist if you just point to it from an Ewk_Notification_Info. You can then rename Ewk_Notification_Info to Ewk_Notification and get rid of some of the unhelpful abstraction.

I agree, the name of structure is not clear, I'll merge Ewk_Notification and Ewk_Notification_Info, and I'll fix my code to your recommendation.

>  * The queueing of callback calls seems flawed (a single callback is kept at a time).

I don't think queuing is needed for callback.
There is a queue in the EventTarget class.

>  * "ewk_notification_{displayed,clicked,closed,error}" are bad names. The last one is not consistent with the rest, and a reader of the API cannot really tell what they do without looking at the code or the documentation (which is also very minimal). "ewk_notification_notify_clicked" etc (or something along those lines) is more helpful (I don't like ewk code calling code in WebCoreSupport, but it seems we can't avoid it this time).

You are right. I will fix like ewk_notification_onclose_event_send, I think these are quite clear.

>  * The granularity of the object called in "notification,show" looks wrong.. A security origin is usually associated with a frame, not a view (a view can have many frames); thus emitting "notification,show" for a view loses some information in the process.

You are right, a security origin is associated with a frame.
But notification have to be showed over the browser or at least over the view, do not display on the frame.
So, I think view object for the smart_callback_call is not a problem.

>  * ewk_notification_permitted_domains_add looks like an ewk_frame function for the reason I mentioned above (in the worst case, an ewk_frame should be passed to this function).
> Some of the analysis may be biased because I was looking at the Qt port's code while reviewing this patch, so other ways to approach these issues are welcome.

I saw the Qt patch for EFL patch also. But, the permission for the notification is working based on the domain.
For instance, there are two frames in the page with same domain, after first frame get a permission for the domain, I think second frame's notifications have to be worked also.
That's why I didn't include frame info. to the ewk_notification_permitted_domains_add() function.

> 
> Last but not least: I don't know how much this patch has been tested in production code, but from the current looks of it, I would _really_ like the notification-related layout tests to pass (which means some small DRT code probably needs to be written) before this is landed. I'm not 100% confident the current code does everything the spec says (for example, I don't see anything related to replaceId), so at least having the tests passing makes me not need to trust my gut feelings that much.

Add replaceId and dir, they were missed.

> 
> > Source/WebKit/efl/CMakeListsEfl.txt:98
> > +IF (ENABLE_NOTIFICATIONS)
> > +    LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> > +        "${WEBCORE_DIR}/notifications"
> > +    )
> > +    LIST(APPEND WebKit_SOURCES
> > +        efl/ewk/ewk_notification.cpp
> > +    )
> > +ENDIF ()
> 
> Why has the block been moved?

I want to add ewk_notification.cpp in the ENABLE_NOTIFICATIONS block.
But, I think It is not good that ewk_notification.cpp is placed before main block of "LIST(APPEND WebKit_SOURCES..".

> 
> > Source/WebKit/efl/CMakeListsEfl.txt:259
> > +    LIST(APPEND EWebKit_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_notification_private.h)
> 
> A private header must not be installed.

You're right. Will be fixed.

> 
> > Source/WebKit/efl/ChangeLog:57
> > +        * ewk/ewk_private.h:
> > +        * ewk/ewk_view.cpp:
> > +        (_ewk_view_priv_new):
> > +        * ewk/ewk_view.h:
> 
> At least the files which have been changed could have some description of what has been changed.

OK.

> 
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:425
> >  #if ENABLE(NOTIFICATIONS)
> >  NotificationPresenter* ChromeClientEfl::notificationPresenter() const
> >  {
> > -    notImplemented();
> > -    return 0;
> > +    return ewk_notification_presenter_get(m_view);
> >  }
> 
> This has been removed from the parent class in r101307, so this method and ewk_notification_presenter_get can be removed altogether.

I agree.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:50
> > +    if (notification->isHTML()) {
> > +        Ewk_Notification_Info* ewkNotificationInfo = ewk_notification_info_create(m_ewkNotification, notification);
> > +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,htmlshow", ewkNotificationInfo);
> > +    } else {
> > +        Ewk_Notification_Info* ewkNotificationInfo = ewk_notification_info_create(m_ewkNotification, notification);
> > +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,show", ewkNotificationInfo);
> > +    }
> 
> Why not have a boolean in the object which tells whether it is an HTML notification or not and then just emit a single type of signal?

Firstly, if I add a boolean field to the Ewk_Notificaion, Object size are grown.(Absolutely this can't be a problem, but I don't like.)
And If separate a signal, a application which is using this API, they doesn't need to check for confirm that is html or not.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-54
> > -    notImplemented();
> 
> Why? This is still not implemented.

Wil be fixed.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:74
> > +    if (m_cachedPermissions.find(context->securityOrigin()->toString()) == m_cachedPermissions.end()) {
> 
> I don't really see why the cached permissions are being searched (mostly because other ports don't do that).

There is find field in the QT port.
323 : QHash<ScriptExecutionContext*, CallbacksInfo >::iterator iter = m_pendingPermissionRequests.find(context);

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:75
> > +        m_callback = callback;
> 
> This method can be called many times and for multiple security origins. How do you handle that with a single variable?
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:76
> > +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,requestPermission" \
> 
> Unneeded "\" at the end of the line.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> > +            , (void*)context->securityOrigin()->toString().utf8().data());
> 
> The C-style cast is not needed, and we do not use C-style casts.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:27
> > +#include <Ecore_Evas.h>
> 
> Why?

Will be fixed.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:43
> > +    virtual void cancelRequestsForPermission(ScriptExecutionContext*) { }
> 
> Doesn't this need to be implemented?

Yes. I think. Notification will be implemented by modal dialog like a chromium, It's impossible to cancel.

> 
> > Source/WebKit/efl/ewk/EWebKit.h:40
> > +#include "ewk_notification.h"
> 
> If ewk_notification.h is not installed, this will break the build for users.

After landing this patch, I think there is no case that ewk_notification.h is not installed.

> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:26
> > +#include "ewk_logging.h"
> 
> Unused.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:37
> > +    WebCore::NotificationPresenterClientEfl* notification_presenter;
> > +    Evas_Object* ewk_view;
> > +    Eina_List* ewk_notification_info_list;
> 
> Wrong naming style.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:41
> > +    Ewk_Notification* ewk_notification;
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:42
> > +    void* notification;
> 
> Why void?
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:47
> > +    unsigned __ref;
> 
> This is not used anywhere and the naming is weird.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:126
> > +Ewk_Notification* ewk_notification_create(WebCore::NotificationPresenter* notificationPresenter, Evas_Object* ewkView)
> > +{
> > +    Ewk_Notification* notification = new Ewk_Notification;
> > +    notification->notification_presenter = static_cast<WebCore::NotificationPresenterClientEfl*>(notificationPresenter);
> 
> Why all this casting if you are already passed a NotificationPresenterClientEfl? Just change the argument type in the function.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:133
> > +void ewk_notification_destroy(Ewk_Notification* ewkNotification)
> 
> What's the point of having this function if it just calls destroy_all? This all could be done directly in the presenter's destructor.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:137
> > +    ewkNotification = 0;
> 
> This has absolutely no effect.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:150
> > +    notification_info->iconURL = strdup(static_cast<WebCore::Notification*>(notification)->contents().icon.string().utf8().data());
> > +    notification_info->title = strdup(static_cast<WebCore::Notification*>(notification)->contents().title.utf8().data());
> > +    notification_info->body = strdup(static_cast<WebCore::Notification*>(notification)->contents().body.utf8().data());
> > +    notification_info->htmlURL = strdup(static_cast<WebCore::Notification*>(notification)->url().string().utf8().data());
> 
> Why not use stringshares here?

The utf8() method in the String class returns temp String object.
That's why I copy values for this.

> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:167
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->iconURL);
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->title);
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->body);
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->htmlURL);
> 
> The checks here are calling free() only if the variables are already 0. Plus, the checks are not needed at all. free(0) is a NOP.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:172
> > +    ewkNotificationInfo = 0;
> 
> This line has no effect.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:175
> > +void ewk_notificaiton_destroy_all(Ewk_Notification* ewkNotification)
> 
> Typo: "notificaiton"
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:23
> > +#include <Evas.h>
> 
> What is the Evas.h include used for? EAPI, for one, is in Eina.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:34
> > + * The following signals (see evas_object_smart_callback_add()) are emitted:
> 
> If there is no signal being emitted, there is no point in having this line.
> 
> Plus, some reference to the HTML5 spec is useful so people know what a web notification is.
I agree.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:44
> > + * Get iconURL for the notification from @c Ewk_Notification_Info.
> 
> "iconURL" is very implementation specific for a documentation entry. Please explain what it means and separate "icon" and "URL" (and in the "@return" line you use "Icon URL", with spaces, different order and different casing.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:48
> > + * @return chacter pointer to the Icon URL of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:49
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:58
> > + * @return chacter pointer to the title of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:59
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:68
> > + * @return chacter pointer to the body contents of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:69
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:74
> > + * Get html contents URL for the notification from @c Ewk_Notification_Info.
> 
> This is not mentioned in the w3c spec, so please explain what "html contents URL" means.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:78
> > + * @return chacter pointer to the html contents url of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:79
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:84
> > + * Calls a callback fucntion which is registerd, after receiving a user approval.
> 
> Typo: "fucntion" and "registerd". And it should be "user's approval".
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:90
> > +EAPI void ewk_notification_permission_callback_call(const Ewk_Notification_Info* eni, Eina_Bool permitted, const char* domain);
> 
> I don't see why this should be called by users. The callback must be called automatically when the user has responded to the permission request.

This function get a user input for the permission of notification.
When user click permit or not for the notification, this function have to be called, 
because there is no return value in the request permission by smart_callback_call("notification,requestPermission").
Will be modify description.

> 
> > Source/WebKit/efl/ewk/ewk_notification.h:93
> > + * Makes a Cache for permitted domains.
> 
> Why is "Cache" capitalized? What should it mean for the reader?
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:95
> > + * This function can be called at initializing time if a application have permited domains.
> 
> Typo: "permited". "initializing" should be "initialization".
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:96
> > + * WebCore checks a permission from the cache only when checkPermission() is called from javascript.
> 
> API users are not expected to know what "WebCore" means.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:102
> > +EAPI void ewk_notification_permitted_domains_add(const Ewk_Notification_Info* eni, char** domains, int size);
> 
> The description above contradicts the code's use: this is currently the only way to add permission for a given security origin and thus cannot be called "at initializing time".

This function is for apps that is use WebKit-EFL APIs.
If the apps have permissions for the notification already, they have to call this function when it is initialized.
Will be add a detail description to ewk_notificaiton.h

> 
> > Source/WebKit/efl/ewk/ewk_notification_private.h:43
> > +void ewk_notificaiton_destroy_all(Ewk_Notification* ewkNotification);
> 
> Typo: "notificaiton"
> 
> > Source/WebKit/efl/ewk/ewk_private.h:-230
> > -
> 
> Unrelated change.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:71
> > +#include "NotificationController.h"
> 
> Is this include really needed?
> 
> > Tools/ChangeLog:9
> > +        And add "WebCore/notifications" source path to build DumpRenderTree.
> 
> No need to say this  if hereyou repeat it below.

[Kihong Kwon]

Created attachment 121963 [details]
Fixed patch for comment #27

[Ryuan Choi]

Comment on attachment 121963 [details]
Fixed patch for comment #27

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

IMO, Approach looks better than before.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:42
> +    HashSet<Ewk_Notification*>::iterator iter = m_pendingNotifications.begin();
> +    while (iter != m_pendingNotifications.end())
> +        ewk_notification_destroy(*iter);

I'm not sure whether we should keep and control Ewk_Notification.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:63
> +    if (notification->isHTML()) {
> +        Ewk_Notification* ewkNotification = ewk_notification_create(this, notification);
> +        evas_object_smart_callback_call(m_view, "notification,htmlshow", ewkNotification);
> +        m_pendingNotifications.add(ewkNotification);
> +    } else {
> +        Ewk_Notification* ewkNotification = ewk_notification_create(this, notification);
> +        evas_object_smart_callback_call(m_view, "notification,show", ewkNotification);
> +        m_pendingNotifications.add(ewkNotification);
> +    }

Instead of controlling m_pendingNotifications, 
why don't you call ewk_notification_deref() after calling evas_object_smart_callback_call()?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:71
> +    evas_object_smart_callback_call(m_view, "notification,cancel", &ewkNotification);

& looks typo, right?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:104
> +        evas_object_smart_callback_call(m_view, "notification,requestPermission"

IMO, "notification,permission,request" is better

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:46
> +    void makePermissionCache(char** domains, int size);
> +    void permittedCallbackCall(const char* domain, const bool permission);

IMO, we'd better to use String and Vector for argument.

> Source/WebKit/efl/ewk/ewk_notification.cpp:32
> +struct _Ewk_Notification {
> +    WebCore::NotificationPresenterClientEfl* notificationPresenter;
> +    WebCore::Notification* notification;

ENABLE(NOTIFICATIONS) is missing.

> Source/WebKit/efl/ewk/ewk_notification.cpp:46
> +char* ewk_notification_icon_url_get(Ewk_Notification* ewkNotification)

How do you think about returning const char* ?

> Source/WebKit/efl/ewk/ewk_notification.cpp:111
> +void ewk_notification_permitted_domains_add(const Ewk_Notification* ewkNotification, char** domains, int size)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    ewkNotification->notificationPresenter->makePermissionCache(domains, size);
> +#endif
> +}

I'm not sure, but char** looks ambiguous.

Why don't you use Eina_List ?

> Source/WebKit/efl/ewk/ewk_notification.cpp:136
> +    delete ewkNotification->iconURL;
> +    delete ewkNotification->title;
> +    delete ewkNotification->body;
> +    delete ewkNotification->htmlURL;
> +    delete ewkNotification->dir;
> +    delete ewkNotification;

You should not delete the memory which assigned by strdup()

> Source/WebKit/efl/ewk/ewk_notification_private.h:31
> +#ifdef __cplusplus
> +extern "C" {
> +#endif

For the internal header, we don't need `extern "C"`

> Source/WebKit/efl/ewk/ewk_view.h:88
> + *  - "notification,requestPermission", char*: requested domain for getting user permission from the user.

char* or const char* ? which one is better ?

[Gyuyoung Kim]

Comment on attachment 121963 [details]
Fixed patch for comment #27

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

> Source/WebKit/efl/ewk/ewk_notification.h:40
> + * Get iconURL for the notification from @c Ewk_Notification.

s/Get/Gets/g

> Source/WebKit/efl/ewk/ewk_notification.h:49
> + * Get title for the notification from @c Ewk_Notification.

s/Get/Gets/g

> Source/WebKit/efl/ewk/ewk_notification.h:55
> +EAPI char* ewk_notification_title_get(Ewk_Notification* eni);

We have added *const* keyword to parameter of _get function. And also, we follow efl coding style in public header file. So, move * to function side.

> Source/WebKit/efl/ewk/ewk_notification.h:64
> +EAPI char* ewk_notification_body_get(Ewk_Notification* eni);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:75
> +EAPI char* ewk_notification_html_url_get(Ewk_Notification* eni);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:84
> +EAPI char* ewk_notification_dir_get(Ewk_Notification* eni);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:114
> +EAPI void ewk_notification_ondisplay_event_send(Ewk_Notification* eni);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:121
> +EAPI void ewk_notification_onclick_event_send(Ewk_Notification* eni);

ditto.

[Kihong Kwon]

Comment on attachment 121963 [details]
Fixed patch for comment #27

We are having an internal discussion. I will submit a new patch.

[Kihong Kwon]

(In reply to comment #30)
> (From update of attachment 121963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121963&action=review
> 
> IMO, Approach looks better than before.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:42
> > +    HashSet<Ewk_Notification*>::iterator iter = m_pendingNotifications.begin();
> > +    while (iter != m_pendingNotifications.end())
> > +        ewk_notification_destroy(*iter);
> 
> I'm not sure whether we should keep and control Ewk_Notification.

I think we have to keep Ewk_Notificaiton for memory management.
If app doesn't send a (onclose/onerror) event and doesn't call a deref, there can be memory leak.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:63
> > +    if (notification->isHTML()) {
> > +        Ewk_Notification* ewkNotification = ewk_notification_create(this, notification);
> > +        evas_object_smart_callback_call(m_view, "notification,htmlshow", ewkNotification);
> > +        m_pendingNotifications.add(ewkNotification);
> > +    } else {
> > +        Ewk_Notification* ewkNotification = ewk_notification_create(this, notification);
> > +        evas_object_smart_callback_call(m_view, "notification,show", ewkNotification);
> > +        m_pendingNotifications.add(ewkNotification);
> > +    }
> 
> Instead of controlling m_pendingNotifications, 
> why don't you call ewk_notification_deref() after calling evas_object_smart_callback_call()?
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:71
> > +    evas_object_smart_callback_call(m_view, "notification,cancel", &ewkNotification);
> 
> & looks typo, right?
Yes.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:104
> > +        evas_object_smart_callback_call(m_view, "notification,requestPermission"
> 
> IMO, "notification,permission,request" is better
I agree.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:46
> > +    void makePermissionCache(char** domains, int size);
> > +    void permittedCallbackCall(const char* domain, const bool permission);
> 
> IMO, we'd better to use String and Vector for argument.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:32
> > +struct _Ewk_Notification {
> > +    WebCore::NotificationPresenterClientEfl* notificationPresenter;
> > +    WebCore::Notification* notification;
> 
> ENABLE(NOTIFICATIONS) is missing.
OK.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:46
> > +char* ewk_notification_icon_url_get(Ewk_Notification* ewkNotification)
> 
> How do you think about returning const char* ?
I think so.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:111
> > +void ewk_notification_permitted_domains_add(const Ewk_Notification* ewkNotification, char** domains, int size)
> > +{
> > +#if ENABLE(NOTIFICATIONS)
> > +    ewkNotification->notificationPresenter->makePermissionCache(domains, size);
> > +#endif
> > +}
> 
> I'm not sure, but char** looks ambiguous.
> 
> Why don't you use Eina_List ?
OK.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:136
> > +    delete ewkNotification->iconURL;
> > +    delete ewkNotification->title;
> > +    delete ewkNotification->body;
> > +    delete ewkNotification->htmlURL;
> > +    delete ewkNotification->dir;
> > +    delete ewkNotification;
> 
> You should not delete the memory which assigned by strdup()
We can use delete with strdup(), but I will change strdup to eina_stringshare_add().
> 
> > Source/WebKit/efl/ewk/ewk_notification_private.h:31
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> 
> For the internal header, we don't need `extern "C"`
OK.
> 
> > Source/WebKit/efl/ewk/ewk_view.h:88
> > + *  - "notification,requestPermission", char*: requested domain for getting user permission from the user.
> 
> char* or const char* ? which one is better ?
we can not use a const char* in the evas_object_smart_callback_call.
In the evas_object_smart_callback_call receive a void* parameter.

[Kihong Kwon]

Created attachment 123891 [details]
Add new patch.

[Kihong Kwon]

Created attachment 123901 [details]
Add new patch.

[Ryuan Choi]

Comment on attachment 123901 [details]
Add new patch.

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

r- on my side.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:103
> +    // Because we are using modal dialogs to request permission, it's impossible to cancel them.

Should we implement this as modal popup?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:106
> +void NotificationPresenterClientEfl::makePermissionCache(Vector<String> domains)

const Vector<String>& looks enough.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:114
> +    for (size_t i = 0; i < m_callbacks.size(); i++) {

{ looks wrong.

> Source/WebKit/efl/ewk/ewk_notification.cpp:63
> +    return ewkNotification->iconURL;

IIRC, iconURL may have trash because you assign values only when icon string is not empty.

> Source/WebKit/efl/ewk/ewk_notification.cpp:155
> +    if (static_cast<WebCore::Notification*>(notification)->replaceId().length())
> +        ewkNotification->iconURL = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->replaceId().utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->contents().icon.string().length())
> +        ewkNotification->iconURL = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->contents().icon.string().utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->contents().title.length())
> +        ewkNotification->title = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->contents().title.utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->contents().body.length())
> +        ewkNotification->body = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->contents().body.utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->url().string().length())
> +        ewkNotification->url = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->url().string().utf8().data());
> +    if (static_cast<WebCore::Notification*>(notification)->dir().length())
> +        ewkNotification->dir = eina_stringshare_add(static_cast<WebCore::Notification*>(notification)
> +            ->dir().utf8().data());

There are many issues.
you don't need to cast, right?

139 and 140 lines is wrong.

> Source/WebKit/efl/ewk/ewk_notification.cpp:183
> +        return;
> +#if ENABLE(NOTIFICATIONS)

empty line please

[Ryuan Choi]

(In reply to comment #36)
> (From update of attachment 123901 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123901&action=review
> > Source/WebKit/efl/ewk/ewk_notification.cpp:63
> > +    return ewkNotification->iconURL;
> 
> IIRC, iconURL may have trash because you assign values only when icon string is not empty.

This comment is wrong, I can't catch ().

But other comments are still valid.

[Kihong Kwon]

Comment on attachment 123901 [details]
Add new patch.

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

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:103
>> +    // Because we are using modal dialogs to request permission, it's impossible to cancel them.
> 
> Should we implement this as modal popup?

I think so.
The permission request has to obtain a user's confirmation immediately before the user performs other operations on that page.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:106
>> +void NotificationPresenterClientEfl::makePermissionCache(Vector<String> domains)
> 
> const Vector<String>& looks enough.

You are right.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:114
>> +    for (size_t i = 0; i < m_callbacks.size(); i++) {
> 
> { looks wrong.

OK.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:63
>> +    return ewkNotification->iconURL;
> 
> IIRC, iconURL may have trash because you assign values only when icon string is not empty.

You already mentioned about this.
Not a problem.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:155
>> +            ->dir().utf8().data());
> 
> There are many issues.
> you don't need to cast, right?
> 
> 139 and 140 lines is wrong.

Yes. There are no needs to cast.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:183
>> +#if ENABLE(NOTIFICATIONS)
> 
> empty line please

OK.

[Kihong Kwon]

Created attachment 124072 [details]
Fixed patch for the comment #31

[Kihong Kwon]

Created attachment 125068 [details]
Merge pending notification queue and stored notification queue.

[Gyuyoung Kim]

Comment on attachment 125068 [details]
Merge pending notification queue and stored notification queue.

Attachment 125068 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11394694

[Kihong Kwon]

Created attachment 125077 [details]
Fix efl build error.

[Kihong Kwon]

Created attachment 145673 [details]
New patch followed by changing WebCore.

[Gyuyoung Kim]

Comment on attachment 145673 [details]
New patch followed by changing WebCore.

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

I think this patch is a little huge. If possible, can you divide this patch into smaller's ?

> LayoutTests/platform/efl/TestExpectations:717
> +// Refactor notification tests requiring file:// permission.

Why do you use // in front of *permission* comment? Do not make unneeded comment style.

> LayoutTests/platform/efl/TestExpectations:726
> +// Efl doesn't support to simulateDesktopNotificationClick.

Efl -> EFL.

> Source/WebCore/ChangeLog:8
> +        Change requestPermission can be processed when argument is null on the JSC.

Change requestPermission -> To change requestPermission ?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:53
> +                    printf("REPLACING NOTIFICATION %s\n", iter->first->isHTML() ? iter->first->url().string().utf8().data() : iter->first->title().utf8().data());

Remove printf(). If you wanna print debug log, use LOG_XXX. Or, if you wanna pass layout test, you should move this printf to DRT side.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:63
> +    evas_object_smart_callback_call(m_view, "notification,show", static_cast<void*>(ewkNotification));

If you add new signal, you should add signal description to ewk_view.h.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:68
> +            printf("DESKTOP NOTIFICATION: contents at %s\n", notification->url().string().utf8().data());

ditto.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:70
> +            printf("DESKTOP NOTIFICATION:%s icon %s, title %s, text %s\n",

ditto.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:90
> +            printf("DESKTOP NOTIFICATION CLOSED: %s\n", notification->url().string().utf8().data());

ditto.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:92
> +            printf("DESKTOP NOTIFICATION CLOSED: %s\n", notification->title().utf8().data());

ditto.

[Gyuyoung Kim]

Comment on attachment 145673 [details]
New patch followed by changing WebCore.

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

>> LayoutTests/platform/efl/TestExpectations:717
>> +// Refactor notification tests requiring file:// permission.
> 
> Why do you use // in front of *permission* comment? Do not make unneeded comment style.

Oops, '//' is for 'file://'. Ignore above comment.

[Kihong Kwon]

Comment on attachment 145673 [details]
New patch followed by changing WebCore.

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

>> LayoutTests/platform/efl/TestExpectations:726
>> +// Efl doesn't support to simulateDesktopNotificationClick.
> 
> Efl -> EFL.

OK.

>> Source/WebCore/ChangeLog:8
>> +        Change requestPermission can be processed when argument is null on the JSC.
> 
> Change requestPermission -> To change requestPermission ?

I don't think so, I meant "Fix to run requestPermission even if arguemnt is empty on the JSC".
I will change this description~;-)

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:53
>> +                    printf("REPLACING NOTIFICATION %s\n", iter->first->isHTML() ? iter->first->url().string().utf8().data() : iter->first->title().utf8().data());
> 
> Remove printf(). If you wanna print debug log, use LOG_XXX. Or, if you wanna pass layout test, you should move this printf to DRT side.

QT port is implemented like this.
In the chromium port, there are additional classes only for the layout tests.
But, I think if I make additional classes for the layout tests, It's not a tests for real implementation. That's why I use printf in the middle of implementation like QT.
How do you think about this?

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:63
>> +    evas_object_smart_callback_call(m_view, "notification,show", static_cast<void*>(ewkNotification));
> 
> If you add new signal, you should add signal description to ewk_view.h.

OK.

[Gyuyoung Kim]

Comment on attachment 145673 [details]
New patch followed by changing WebCore.

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

>>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:53
>>> +                    printf("REPLACING NOTIFICATION %s\n", iter->first->isHTML() ? iter->first->url().string().utf8().data() : iter->first->title().utf8().data());
>> 
>> Remove printf(). If you wanna print debug log, use LOG_XXX. Or, if you wanna pass layout test, you should move this printf to DRT side.
> 
> QT port is implemented like this.
> In the chromium port, there are additional classes only for the layout tests.
> But, I think if I make additional classes for the layout tests, It's not a tests for real implementation. That's why I use printf in the middle of implementation like QT.
> How do you think about this?

If you follow QT implementation, I don't mind to do it. However, this patch is huge, I feel difficult to review this detail.

[Kihong Kwon]

(In reply to comment #47)
> (From update of attachment 145673 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145673&action=review
> 
> >>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:53
> >>> +                    printf("REPLACING NOTIFICATION %s\n", iter->first->isHTML() ? iter->first->url().string().utf8().data() : iter->first->title().utf8().data());
> >> 
> >> Remove printf(). If you wanna print debug log, use LOG_XXX. Or, if you wanna pass layout test, you should move this printf to DRT side.
> > 
> > QT port is implemented like this.
> > In the chromium port, there are additional classes only for the layout tests.
> > But, I think if I make additional classes for the layout tests, It's not a tests for real implementation. That's why I use printf in the middle of implementation like QT.
> > How do you think about this?
> 
> If you follow QT implementation, I don't mind to do it. However, this patch is huge, I feel difficult to review this detail.

OK, I will try to divide this patch~:-)

[Kihong Kwon]

Created attachment 145701 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 145701 [details]
Patch.

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

Please read WebKit EFL coding style guideline.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:74
> +    // Called from ~Notification(), Remove the entry from the notifications list.

s/Remove/remove/g

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:45
> +    virtual void requestPermission(ScriptExecutionContext*, PassRefPtr<NotificationPermissionCallback>) { }

I couldn't find implementation for this virtual function. Don't you need to implement this ?

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

Is it better to add comment for these internal functions ?

> Source/WebKit/efl/ewk/ewk_notification.h:38
> +    char* domain;

Move '*' to variable side.

> Source/WebKit/efl/ewk/ewk_notification.h:39
> +    bool isAllowed;

s/isAllowed/allow/g. In addition, use Eina_Bool instead of bool.

> Source/WebKit/efl/ewk/ewk_notification.h:49
> +EAPI void ewk_notification_dispatch_click(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:56
> +EAPI void ewk_notification_dispatch_close(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:63
> +EAPI void ewk_notification_dispatch_error(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:70
> +EAPI void ewk_notification_dispatch_show(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:79
> +EAPI Eina_Stringshare* ewk_notification_title_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:82
> + * Get icon url from @c Ewk_Notification.

s/url/URL/g.

> Source/WebKit/efl/ewk/ewk_notification.h:86
> + * @return icon url of the notification which has to be deleted by eina_stringshare_del after using it.

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:88
> +EAPI Eina_Stringshare* ewk_notification_icon_url_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:98
> +EAPI Eina_Stringshare* ewk_notification_body_get(Ewk_Notification* en);
> +

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:100
> + * Get tag/replaced id from @c Ewk_Notification.

I don't understand what does "tag/replaed" mean ?

> Source/WebKit/efl/ewk/ewk_notification.h:106
> +EAPI Eina_Stringshare* ewk_notification_tag_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:115
> +EAPI Eina_Stringshare* ewk_notification_dir_get(Ewk_Notification* en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification_private.h:28
> +WebCore::Notification* ewk_notification_get_notification(Ewk_Notification* ewkNotification);

Naming violation. ewk_notification_notification_get(). If you worry about duplicating naming, I think you can modify name more meaning thing.

> Source/WebKit/efl/ewk/ewk_notification_private.h:33
> +

Unneeded line.

> Source/WebKit/efl/ewk/ewk_view.cpp:4381
> +void ewk_view_notification_permissions_set(Evas_Object* ewkView, const char* domain, Eina_Bool isAllowed)

s/isAllowed/allow/g.

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

ditto.

> Source/WebKit/efl/ewk/ewk_view.h:76
> + *  - "notification,show", Ewk_Notification*: request to show notification.

List this signal up by alphabetical order.

> Source/WebKit/efl/ewk/ewk_view.h:2638
> +EAPI void ewk_view_notification_permissions_set(Evas_Object* o, const char* domain, Eina_Bool isAllowed);

Change isAllowed variable with allow. In addition, move '*' operator to variable side.

> Source/WebKit/efl/ewk/ewk_view_private.h:158
> +WebCore::NotificationPresenterClientEfl* ewk_view_notification_client_get(Evas_Object* ewkView);

Add *const* keyword for _get() function.

[Kihong Kwon]

Comment on attachment 145701 [details]
Patch.

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

Thank you for your kind comments. Gyuyoung~:-)

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:74
>> +    // Called from ~Notification(), Remove the entry from the notifications list.
> 
> s/Remove/remove/g

OK.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:45
>> +    virtual void requestPermission(ScriptExecutionContext*, PassRefPtr<NotificationPermissionCallback>) { }
> 
> I couldn't find implementation for this virtual function. Don't you need to implement this ?

There are no implementation in the chromium and qt also.
This function is for W3C web notification spec, but we are support legacy notification until now like chromium and qt.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:32
>> +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> 
> Is it better to add comment for these internal functions ?

OK.

>> Source/WebKit/efl/ewk/ewk_notification.h:38
>> +    char* domain;
> 
> Move '*' to variable side.

OK.

>> Source/WebKit/efl/ewk/ewk_notification.h:100
>> + * Get tag/replaced id from @c Ewk_Notification.
> 
> I don't understand what does "tag/replaed" mean ?

I will change this to replaceId.(followed by standard.)

>> Source/WebKit/efl/ewk/ewk_notification_private.h:28
>> +WebCore::Notification* ewk_notification_get_notification(Ewk_Notification* ewkNotification);
> 
> Naming violation. ewk_notification_notification_get(). If you worry about duplicating naming, I think you can modify name more meaning thing.

OK.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4381
>> +void ewk_view_notification_permissions_set(Evas_Object* ewkView, const char* domain, Eina_Bool isAllowed)
> 
> s/isAllowed/allow/g.

OK.

>> Source/WebKit/efl/ewk/ewk_view.h:76
>> + *  - "notification,show", Ewk_Notification*: request to show notification.
> 
> List this signal up by alphabetical order.

OK.

>> Source/WebKit/efl/ewk/ewk_view_private.h:158
>> +WebCore::NotificationPresenterClientEfl* ewk_view_notification_client_get(Evas_Object* ewkView);
> 
> Add *const* keyword for _get() function.

OK.

[Kihong Kwon]

Created attachment 145722 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 145722 [details]
Patch.

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

> Source/WebKit/efl/ChangeLog:9
> +        In this patch, we support following apis and events.

s/apis/APIs/g

> Source/WebKit/efl/ewk/ewk_notification.cpp:91
> +Eina_Stringshare* ewk_notification_title_get(Ewk_Notification* ewkNotification)

Add const keyword for _get().

> Source/WebKit/efl/ewk/ewk_notification.cpp:100
> +Eina_Stringshare* ewk_notification_icon_url_get(Ewk_Notification* ewkNotification)

ditto.

> Source/WebKit/efl/ewk/ewk_notification.cpp:109
> +Eina_Stringshare* ewk_notification_body_get(Ewk_Notification* ewkNotification)

ditto.

> Source/WebKit/efl/ewk/ewk_notification.cpp:118
> +Eina_Stringshare* ewk_notification_replace_id_get(Ewk_Notification* ewkNotification)

ditto.

> Source/WebKit/efl/ewk/ewk_notification.cpp:127
> +Eina_Stringshare* ewk_notification_dir_get(Ewk_Notification* ewkNotification)

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:23
> + */

Please write more detail description.

> Source/WebKit/efl/ewk/ewk_notification.h:88
> +EAPI Eina_Stringshare *ewk_notification_icon_url_get(Ewk_Notification *en);

By the way,  why we have to return string by Eina_Stringshare ? As you know, application have to delete returned string by eina function. EFL port has used const char* for similar case.

> Source/WebKit/efl/ewk/ewk_notification_private.h:32
> +#endif // ewk_settings_private_h

s/ewk_settings_private_h/ewk_notification_private.h/g

> Source/WebKit/efl/ewk/ewk_view.cpp:4391
> +const WebCore::NotificationPresenterClientEfl* ewk_view_notification_client_get(Evas_Object* ewkView)

Missing const keyword.

[Kihong Kwon]

Comment on attachment 145722 [details]
Patch.

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

>> Source/WebKit/efl/ChangeLog:9
>> +        In this patch, we support following apis and events.
> 
> s/apis/APIs/g

OK.

>> Source/WebKit/efl/ewk/ewk_notification.cpp:91
>> +Eina_Stringshare* ewk_notification_title_get(Ewk_Notification* ewkNotification)
> 
> Add const keyword for _get().

OK.

>> Source/WebKit/efl/ewk/ewk_notification.h:23
>> + */
> 
> Please write more detail description.

OK.

>> Source/WebKit/efl/ewk/ewk_notification.h:88
>> +EAPI Eina_Stringshare *ewk_notification_icon_url_get(Ewk_Notification *en);
> 
> By the way,  why we have to return string by Eina_Stringshare ? As you know, application have to delete returned string by eina function. EFL port has used const char* for similar case.

OK. I don't care about that. I will change Eina_Stringshare to cont cnar*.

>> Source/WebKit/efl/ewk/ewk_notification_private.h:32
>> +#endif // ewk_settings_private_h
> 
> s/ewk_settings_private_h/ewk_notification_private.h/g

OK.

>> Source/WebKit/efl/ewk/ewk_view.cpp:4391
>> +const WebCore::NotificationPresenterClientEfl* ewk_view_notification_client_get(Evas_Object* ewkView)
> 
> Missing const keyword.

OK.

[Kihong Kwon]

Created attachment 146824 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 146824 [details]
Patch.

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

> Source/WebKit/PlatformEfl.cmake:79
>  IF (ENABLE_NOTIFICATIONS)

I think you can add this to source list unconditionally because ewk_notification.cpp is already protected by #ifdef.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:48
> +            if (iter->first->tag() == notification->tag() && iter->first->url().protocol() == notification->url().protocol() && iter->first->url().host() == notification->url().host()) {

It looks it is better to use multiple lines for this conditions.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:91
> +    evas_object_smart_callback_call(m_view, "notification,permission,request", static_cast<void*>(const_cast<char*>(domain.utf8().data())));

I'm not sure whether multicasting is safe like this. EFL port has used (void*) C-type casting instead of C++ casting for this.

I think you can add internal functions to send signal to ewk_notificaiton.cpp

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:113
> +        evas_object_smart_callback_call(m_view, "notification,permission,cancel", static_cast<void*>(const_cast<char*>(domain.utf8().data())));

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:83
> +EAPI const char *ewk_notification_title_get(Ewk_Notification *en);

As mentioned in comment #53, _get() needs to use *const*.

> Source/WebKit/efl/ewk/ewk_notification.h:92
> +EAPI const char *ewk_notification_icon_url_get(Ewk_Notification *en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:101
> +EAPI const char *ewk_notification_body_get(Ewk_Notification *en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:110
> +EAPI const char *ewk_notification_replace_id_get(Ewk_Notification *en);

ditto.

> Source/WebKit/efl/ewk/ewk_notification.h:119
> +EAPI const char *ewk_notification_dir_get(Ewk_Notification *en);

ditto.

> Tools/ChangeLog:9
> +

Missing changed file in ChangeLog.

[Kihong Kwon]

Comment on attachment 146824 [details]
Patch.

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

>> Source/WebKit/PlatformEfl.cmake:79
>>  IF (ENABLE_NOTIFICATIONS)
> 
> I think you can add this to source list unconditionally because ewk_notification.cpp is already protected by #ifdef.

OK.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:48
>> +            if (iter->first->tag() == notification->tag() && iter->first->url().protocol() == notification->url().protocol() && iter->first->url().host() == notification->url().host()) {
> 
> It looks it is better to use multiple lines for this conditions.

OK.

>> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:91
>> +    evas_object_smart_callback_call(m_view, "notification,permission,request", static_cast<void*>(const_cast<char*>(domain.utf8().data())));
> 
> I'm not sure whether multicasting is safe like this. EFL port has used (void*) C-type casting instead of C++ casting for this.
> 
> I think you can add internal functions to send signal to ewk_notificaiton.cpp

OK.

>> Source/WebKit/efl/ewk/ewk_notification.h:83
>> +EAPI const char *ewk_notification_title_get(Ewk_Notification *en);
> 
> As mentioned in comment #53, _get() needs to use *const*.

OK.

>> Tools/ChangeLog:9
>> +
> 
> Missing changed file in ChangeLog.

OK.

[Kihong Kwon]

Created attachment 148060 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 148060 [details]
Patch.

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

Could you let me know what is difference between NOTIFICATION feature and LEGACY_NOTIFICATIONS? It is a little difficult to understand the difference in this patch.

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-24
> -#include "NotImplemented.h"

When NOTIFICATION is disabled, there is build break as below,

86%] Building CXX object Source/WebKit/CMakeFiles/ewebkit.dir/efl/WebCoreSupport/PageClientEfl.cpp.o
In file included from /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:21:0:
/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:24:26: fatal error: Notification.h: No such file or directory

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> +    // Called from ~Notification(), Remove the entry from the notifications list.

s/Remove/remove/g

> Source/WebKit/efl/ewk/ewk_notification.cpp:131
> +    return strdup(ewkNotification->notification->replaceId().utf8().data());

When LEGACY_NOTIFICATION is disabled, there is a build break. We have to support perfect build regardless of feature dependency.

/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/ewk/ewk_notification.cpp: In function ‘const char* ewk_notification_replace_id_get(const Ewk_Notification*)’:
/home/gyuyoung/webkit/WebKit/Source/WebKit/efl/ewk/ewk_notification.cpp:131:50: error: ‘class WebCore::Notification’ has no member named ‘replaceId’
[ 99%] Built target ewebkit2

[Kihong Kwon]

(In reply to comment #59)
> (From update of attachment 148060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148060&action=review
> 
> Could you let me know what is difference between NOTIFICATION feature and LEGACY_NOTIFICATIONS? It is a little difficult to understand the difference in this patch.

NOTIFICATION support to the Web Notification of w3c otherwise, LEGACY_NOTIFICATION support to the old version of the Web Notification.
NOTIFICATION(http://dvcs.w3.org/hg/notifications/raw-file/tip/Overview.html)
LEGACY_NOTIFICATION(http://www.chromium.org/developers/design-documents/desktop-notifications/api-specification)
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-24
> > -#include "NotImplemented.h"
> 
> When NOTIFICATION is disabled, there is build break as below,
> 
> 86%] Building CXX object Source/WebKit/CMakeFiles/ewebkit.dir/efl/WebCoreSupport/PageClientEfl.cpp.o
> In file included from /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:21:0:
> /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:24:26: fatal error: Notification.h: No such file or directory
> 
LEGACY_NOTIFICATION is like a optional feature of NOTIFICATION, therefore there is no chance to enable LEGACY_NOTIFICATION without NOTIFICAION.
(there are some link errors from WebCore by WebCore/CMakeLIsts.txt:1703)
But I will change this.

> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> > +    // Called from ~Notification(), Remove the entry from the notifications list.
> 
> s/Remove/remove/g

OK.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:131
> > +    return strdup(ewkNotification->notification->replaceId().utf8().data());
> 
> When LEGACY_NOTIFICATION is disabled, there is a build break. We have to support perfect build regardless of feature dependency.
> 
> /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/ewk/ewk_notification.cpp: In function ‘const char* ewk_notification_replace_id_get(const Ewk_Notification*)’:
> /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/ewk/ewk_notification.cpp:131:50: error: ‘class WebCore::Notification’ has no member named ‘replaceId’
> [ 99%] Built target ewebkit2

OK, You are right.

[Kihong Kwon]

Created attachment 148240 [details]
Patch

> > > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-24
> > > -#include "NotImplemented.h"
> > 
> > When NOTIFICATION is disabled, there is build break as below,
> > 
> > 86%] Building CXX object Source/WebKit/CMakeFiles/ewebkit.dir/efl/WebCoreSupport/PageClientEfl.cpp.o
> > In file included from /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:21:0:
> > /home/gyuyoung/webkit/WebKit/Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:24:26: fatal error: Notification.h: No such file or directory
> > 
> LEGACY_NOTIFICATION is like a optional feature of NOTIFICATION, therefore there is no chance to enable LEGACY_NOTIFICATION without NOTIFICAION.
> (there are some link errors from WebCore by WebCore/CMakeLIsts.txt:1703)
> But I will change this.

If I fix this, we need to change position of "${WEBCORE_DIR}/notifications" in the platformEfl.txt

From : 
IF (ENABLE_NOTIFICATIONS)
  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
    "${WEBCORE_DIR}/notifications"
  )
ENDIF ()

To :
  LIST(APPEND WebKit_INCLUDE_DIRECTORIES
    ...
    "${WEBCORE_DIR}/notifications"
    ...
  )

But, there are remaining errors in the WebCore as I said above.
Therefore, I don't want to change this now.
When ENABLE_NOTIFICATION feature is deleted for enabling by default, we can change this.

[Gyuyoung Kim]

Comment on attachment 148240 [details]
Patch

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

It looks there is no critical problems now.

> Source/WebKit/efl/ewk/ewk_notification.h:29
> +/**

Generally, we have written this file description just below licence.

[Kihong Kwon]

(In reply to comment #62)
> (From update of attachment 148240 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148240&action=review
> 
> It looks there is no critical problems now.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:29
> > +/**
> 
> Generally, we have written this file description just below licence.

OK. I will change it.
Thanks for reviewing.

[Gyuyoung Kim]

NotificationPresenterClientEFl was changed with NotificationClientEfl on r121679.
http://trac.webkit.org/changeset/121679

This patch needs to be updated based on latest WebKit.

[Kihong Kwon]

Comment on attachment 148240 [details]
Patch

I will divide this patch with respect to the functionality.

[Thiago Marcos P. Santos]

Is there any work going on for EFL WK2?

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