Summary: | [EFL] Change class name from NotificationClientEfl to NotificationPresenterClientEfl. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kihong Kwon <kihong.kwon> | ||||||||
Component: | WebKit EFL | Assignee: | 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
Kihong Kwon
2012-07-04 04:01:33 PDT
Created attachment 150768 [details]
Patch
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*. (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. Informal rs+ on my side. 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? (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. Created attachment 150902 [details]
Patch
Created attachment 150904 [details]
Patch
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.
(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~ :-) Comment on attachment 150904 [details] Patch Clearing flags on attachment: 150904 Committed r121898: <http://trac.webkit.org/changeset/121898> All reviewed patches have been landed. Closing bug. |