Bug 90542 - [EFL] Change class name from NotificationClientEfl to NotificationPresenterClientEfl.
Summary: [EFL] Change class name from NotificationClientEfl to NotificationPresenterCl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 73544
  Show dependency tree / graph
 
Reported: 2012-07-04 04:01 PDT by Kihong Kwon
Modified: 2012-07-05 03:54 PDT (History)
6 users (show)

See Also:

Attachments
Patch (12.46 KB, patch)
2012-07-04 04:23 PDT, Kihong Kwon 		no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Kihong Kwon 2012-07-04 04:01:33 PDT 
Change class name from NotificationClientEfl to NotificationPresenterClientEfl like all other ports(qt, chromium, blackberry).
Comment 1 Kihong Kwon 2012-07-04 04:23:23 PDT 
Created attachment 150768 [details]
Patch
Comment 2 Gyuyoung Kim 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*.
Comment 3 Kihong Kwon 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.
Comment 4 Gyuyoung Kim 2012-07-04 22:55:51 PDT 
Informal rs+ on my side.
Comment 5 Kenneth Rohde Christiansen 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?
Comment 6 Kihong Kwon 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.
Comment 7 Kihong Kwon 2012-07-05 02:31:11 PDT 
Created attachment 150902 [details]
Patch
Comment 8 Kihong Kwon 2012-07-05 02:36:00 PDT 
Created attachment 150904 [details]
Patch
Comment 9 Gyuyoung Kim 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.
Comment 10 Kihong Kwon 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~ :-)
Comment 11 WebKit Review Bot 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>
Comment 12 WebKit Review Bot 2012-07-05 03:54:27 PDT 
All reviewed patches have been landed.  Closing bug.