Bug 90542

Summary: [EFL] Change class name from NotificationClientEfl to NotificationPresenterClientEfl.
Product: WebKit Reporter: Kihong Kwon <kihong.kwon>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, gyuyoung.kim, lucas.de.marchi, rakuco, vimff0, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 73544    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Kihong Kwon
Reported 2012-07-04 04:01:33 PDT
Change class name from NotificationClientEfl to NotificationPresenterClientEfl like all other ports(qt, chromium, blackberry).
Attachments
Patch (12.46 KB, patch)
2012-07-04 04:23 PDT, Kihong Kwon 		no flags
DetailsFormatted DiffDiff
Patch (12.53 KB, text/plain)
2012-07-05 02:31 PDT, Kihong Kwon 		no flags
Details
Patch (12.56 KB, patch)
2012-07-05 02:36 PDT, Kihong Kwon 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Kihong Kwon
Comment 1 2012-07-04 04:23:23 PDT
Created attachment 150768 [details] Patch
Gyuyoung Kim
Comment 2 2012-07-04 21:45:00 PDT
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
Comment 3 2012-07-04 22:20:09 PDT
(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
Comment 4 2012-07-04 22:55:51 PDT
Informal rs+ on my side.
Kenneth Rohde Christiansen
Comment 5 2012-07-05 01:43:42 PDT
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
Comment 6 2012-07-05 02:13:09 PDT
(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
Comment 7 2012-07-05 02:31:11 PDT
Created attachment 150902 [details] Patch
Kihong Kwon
Comment 8 2012-07-05 02:36:00 PDT
Created attachment 150904 [details] Patch
Gyuyoung Kim
Comment 9 2012-07-05 03:03:24 PDT
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
Comment 10 2012-07-05 03:12:49 PDT
(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 11 2012-07-05 03:54:21 PDT
Comment on attachment 150904 [details] Patch Clearing flags on attachment: 150904 Committed r121898: <http://trac.webkit.org/changeset/121898>
WebKit Review Bot
Comment 12 2012-07-05 03:54:27 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.