[Webkit-unassigned] [Bug 73544] [EFL] Add Web Notification feature for Efl port

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Kihong Kwon <kihong.kwon at samsung.com>  2011-12-14 03:12:58 PST ---
> > 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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.