Bug 90542 – [EFL] Change class name from NotificationClientEfl to NotificationPresenterClientEfl.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kihong Kwon]

Change class name from NotificationClientEfl to NotificationPresenterClientEfl like all other ports(qt, chromium, blackberry).

[Kihong Kwon]

Created attachment 150768 [details]
Patch

[Gyuyoung Kim]

Should we obstinately change existing file name with this one? Mac port is using NotificationClient. But, I don't object to use NotificaitonPresenterClientEfl if there is meaning in *Presenter*.

[Kihong Kwon]

(In reply to comment #2)
> Should we obstinately change existing file name with this one? Mac port is using NotificationClient. But, I don't object to use NotificaitonPresenterClientEfl if there is meaning in *Presenter*.

I guess the meaning of presenter class is that it is a class for presenting the notification.
If nobody has a problem with this, I would like to change the class name like chromium and qt port because my implementation follows the chromium and qt port as a reference.

[Gyuyoung Kim]

Informal rs+ on my side.

[Kenneth Rohde Christiansen]

Comment on attachment 150768 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * PlatformEfl.cmake: Change file name from NotificationClientEfl.cpp to NotificationPresenterClientEfl.cpp

Would be nice with some reasoning for this change, like "to be in line with other ports"

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:24
> +
> +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> +#include "Notification.h"

Shouldn't we instead remove support for legacy notifications?

[Kihong Kwon]

(In reply to comment #5)
> (From update of attachment 150768 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150768&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * PlatformEfl.cmake: Change file name from NotificationClientEfl.cpp to NotificationPresenterClientEfl.cpp
> 
> Would be nice with some reasoning for this change, like "to be in line with other ports"

OK.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:24
> > +
> > +#if ENABLE(NOTIFICATIONS) || ENABLE(LEGACY_NOTIFICATIONS)
> > +#include "Notification.h"
> 
> Shouldn't we instead remove support for legacy notifications?

Actually, legacy notification is used on the web for now. And I think we can remove legacy notification on the same time with other ports.
If there is not a problem to support legacy notification, I would like to support that also.

[Kihong Kwon]

Created attachment 150902 [details]
Patch

[Kihong Kwon]

Created attachment 150904 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 150904 [details]
Patch

Kihong, if you already get r+,you don't need to get review again. 

I also think legacy notification should be solved by other bug.

[Kihong Kwon]

(In reply to comment #9)
> (From update of attachment 150904 [details])
> Kihong, if you already get r+,you don't need to get review again. 
> 
> I also think legacy notification should be solved by other bug.

Thanks. Gyuyoung~ :-)

[WebKit Review Bot]

Comment on attachment 150904 [details]
Patch

Clearing flags on attachment: 150904

Committed r121898: <http://trac.webkit.org/changeset/121898>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.