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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 10 19:28:09 PST 2012


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





--- Comment #28 from Kihong Kwon <kihong.kwon at samsung.com>  2012-01-10 19:28:09 PST ---
Firstly Thank you for your comment. Kubo.

(In reply to comment #27)
> (From update of attachment 121414 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121414&action=review
> 
> This still needs *a lot* of love before getting into trunk.
> 
> Starting with the basics: there are more English mistakes than usual, the documentation is not helpful at all for some random API user who comes across this part of ewk for the first time, there are unused #includes in many files, EWebKit.h will break if ewk_notification.h is not installed, there are many typos and useless statements (eg. void foo(MyObject* ptr) { /* do something */ ptr = 0; }). If possible, I highly recommend doing some internal reviews to spot all these mistakes before sending the bug upstream, so at least some back-and-forth is avoided and reviewers can concentrate on the real issues.
> 
> Now for the real issues (many parts have been commented inline): this is over-engineered and the architecture is not very good.
>  * Let's start with the distinction between Ewk_Notification and Ewk_Notification_Info:
>    - The names chose are really unhelpful. According to the spec (and, to some extent, the API in WebCore), a Notification is a single notificaton shown to the user. Withing ewk, a Notification actually corresponds to an Ewk_Notification_Info, and Ewk_Notification is a poor man's controller.
>    - Ewk_Notification does not really need to exist. Of its 3 attributes, ewk_view is not needed (just store the view in NotificationPresenterEfl's constructor), ewk_notification_info_list can be stored in the presenter itself (using a WTF data structure) and notification_presenter does not need to exist if you just point to it from an Ewk_Notification_Info. You can then rename Ewk_Notification_Info to Ewk_Notification and get rid of some of the unhelpful abstraction.

I agree, the name of structure is not clear, I'll merge Ewk_Notification and Ewk_Notification_Info, and I'll fix my code to your recommendation.

>  * The queueing of callback calls seems flawed (a single callback is kept at a time).

I don't think queuing is needed for callback.
There is a queue in the EventTarget class.

>  * "ewk_notification_{displayed,clicked,closed,error}" are bad names. The last one is not consistent with the rest, and a reader of the API cannot really tell what they do without looking at the code or the documentation (which is also very minimal). "ewk_notification_notify_clicked" etc (or something along those lines) is more helpful (I don't like ewk code calling code in WebCoreSupport, but it seems we can't avoid it this time).

You are right. I will fix like ewk_notification_onclose_event_send, I think these are quite clear.

>  * The granularity of the object called in "notification,show" looks wrong.. A security origin is usually associated with a frame, not a view (a view can have many frames); thus emitting "notification,show" for a view loses some information in the process.

You are right, a security origin is associated with a frame.
But notification have to be showed over the browser or at least over the view, do not display on the frame.
So, I think view object for the smart_callback_call is not a problem.

>  * ewk_notification_permitted_domains_add looks like an ewk_frame function for the reason I mentioned above (in the worst case, an ewk_frame should be passed to this function).
> Some of the analysis may be biased because I was looking at the Qt port's code while reviewing this patch, so other ways to approach these issues are welcome.

I saw the Qt patch for EFL patch also. But, the permission for the notification is working based on the domain.
For instance, there are two frames in the page with same domain, after first frame get a permission for the domain, I think second frame's notifications have to be worked also.
That's why I didn't include frame info. to the ewk_notification_permitted_domains_add() function.

> 
> Last but not least: I don't know how much this patch has been tested in production code, but from the current looks of it, I would _really_ like the notification-related layout tests to pass (which means some small DRT code probably needs to be written) before this is landed. I'm not 100% confident the current code does everything the spec says (for example, I don't see anything related to replaceId), so at least having the tests passing makes me not need to trust my gut feelings that much.

Add replaceId and dir, they were missed.

> 
> > Source/WebKit/efl/CMakeListsEfl.txt:98
> > +IF (ENABLE_NOTIFICATIONS)
> > +    LIST(APPEND WebKit_INCLUDE_DIRECTORIES
> > +        "${WEBCORE_DIR}/notifications"
> > +    )
> > +    LIST(APPEND WebKit_SOURCES
> > +        efl/ewk/ewk_notification.cpp
> > +    )
> > +ENDIF ()
> 
> Why has the block been moved?

I want to add ewk_notification.cpp in the ENABLE_NOTIFICATIONS block.
But, I think It is not good that ewk_notification.cpp is placed before main block of "LIST(APPEND WebKit_SOURCES..".

> 
> > Source/WebKit/efl/CMakeListsEfl.txt:259
> > +    LIST(APPEND EWebKit_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/efl/ewk/ewk_notification_private.h)
> 
> A private header must not be installed.

You're right. Will be fixed.

> 
> > Source/WebKit/efl/ChangeLog:57
> > +        * ewk/ewk_private.h:
> > +        * ewk/ewk_view.cpp:
> > +        (_ewk_view_priv_new):
> > +        * ewk/ewk_view.h:
> 
> At least the files which have been changed could have some description of what has been changed.

OK.

> 
> > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:425
> >  #if ENABLE(NOTIFICATIONS)
> >  NotificationPresenter* ChromeClientEfl::notificationPresenter() const
> >  {
> > -    notImplemented();
> > -    return 0;
> > +    return ewk_notification_presenter_get(m_view);
> >  }
> 
> This has been removed from the parent class in r101307, so this method and ewk_notification_presenter_get can be removed altogether.

I agree.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:50
> > +    if (notification->isHTML()) {
> > +        Ewk_Notification_Info* ewkNotificationInfo = ewk_notification_info_create(m_ewkNotification, notification);
> > +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,htmlshow", ewkNotificationInfo);
> > +    } else {
> > +        Ewk_Notification_Info* ewkNotificationInfo = ewk_notification_info_create(m_ewkNotification, notification);
> > +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,show", ewkNotificationInfo);
> > +    }
> 
> Why not have a boolean in the object which tells whether it is an HTML notification or not and then just emit a single type of signal?

Firstly, if I add a boolean field to the Ewk_Notificaion, Object size are grown.(Absolutely this can't be a problem, but I don't like.)
And If separate a signal, a application which is using this API, they doesn't need to check for confirm that is html or not.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-54
> > -    notImplemented();
> 
> Why? This is still not implemented.

Wil be fixed.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:74
> > +    if (m_cachedPermissions.find(context->securityOrigin()->toString()) == m_cachedPermissions.end()) {
> 
> I don't really see why the cached permissions are being searched (mostly because other ports don't do that).

There is find field in the QT port.
323 : QHash<ScriptExecutionContext*, CallbacksInfo >::iterator iter = m_pendingPermissionRequests.find(context);

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:75
> > +        m_callback = callback;
> 
> This method can be called many times and for multiple security origins. How do you handle that with a single variable?
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:76
> > +        evas_object_smart_callback_call(ewk_notification_view_get(m_ewkNotification), "notification,requestPermission" \
> 
> Unneeded "\" at the end of the line.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:77
> > +            , (void*)context->securityOrigin()->toString().utf8().data());
> 
> The C-style cast is not needed, and we do not use C-style casts.
> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:27
> > +#include <Ecore_Evas.h>
> 
> Why?

Will be fixed.

> 
> > Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:43
> > +    virtual void cancelRequestsForPermission(ScriptExecutionContext*) { }
> 
> Doesn't this need to be implemented?

Yes. I think. Notification will be implemented by modal dialog like a chromium, It's impossible to cancel.

> 
> > Source/WebKit/efl/ewk/EWebKit.h:40
> > +#include "ewk_notification.h"
> 
> If ewk_notification.h is not installed, this will break the build for users.

After landing this patch, I think there is no case that ewk_notification.h is not installed.

> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:26
> > +#include "ewk_logging.h"
> 
> Unused.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:37
> > +    WebCore::NotificationPresenterClientEfl* notification_presenter;
> > +    Evas_Object* ewk_view;
> > +    Eina_List* ewk_notification_info_list;
> 
> Wrong naming style.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:41
> > +    Ewk_Notification* ewk_notification;
> 
> Ditto.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:42
> > +    void* notification;
> 
> Why void?
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:47
> > +    unsigned __ref;
> 
> This is not used anywhere and the naming is weird.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:126
> > +Ewk_Notification* ewk_notification_create(WebCore::NotificationPresenter* notificationPresenter, Evas_Object* ewkView)
> > +{
> > +    Ewk_Notification* notification = new Ewk_Notification;
> > +    notification->notification_presenter = static_cast<WebCore::NotificationPresenterClientEfl*>(notificationPresenter);
> 
> Why all this casting if you are already passed a NotificationPresenterClientEfl? Just change the argument type in the function.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:133
> > +void ewk_notification_destroy(Ewk_Notification* ewkNotification)
> 
> What's the point of having this function if it just calls destroy_all? This all could be done directly in the presenter's destructor.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:137
> > +    ewkNotification = 0;
> 
> This has absolutely no effect.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:150
> > +    notification_info->iconURL = strdup(static_cast<WebCore::Notification*>(notification)->contents().icon.string().utf8().data());
> > +    notification_info->title = strdup(static_cast<WebCore::Notification*>(notification)->contents().title.utf8().data());
> > +    notification_info->body = strdup(static_cast<WebCore::Notification*>(notification)->contents().body.utf8().data());
> > +    notification_info->htmlURL = strdup(static_cast<WebCore::Notification*>(notification)->url().string().utf8().data());
> 
> Why not use stringshares here?

The utf8() method in the String class returns temp String object.
That's why I copy values for this.

> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:167
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->iconURL);
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->title);
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->body);
> > +    if (!ewkNotificationInfo->iconURL)
> > +        free(ewkNotificationInfo->htmlURL);
> 
> The checks here are calling free() only if the variables are already 0. Plus, the checks are not needed at all. free(0) is a NOP.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:172
> > +    ewkNotificationInfo = 0;
> 
> This line has no effect.
> 
> > Source/WebKit/efl/ewk/ewk_notification.cpp:175
> > +void ewk_notificaiton_destroy_all(Ewk_Notification* ewkNotification)
> 
> Typo: "notificaiton"
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:23
> > +#include <Evas.h>
> 
> What is the Evas.h include used for? EAPI, for one, is in Eina.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:34
> > + * The following signals (see evas_object_smart_callback_add()) are emitted:
> 
> If there is no signal being emitted, there is no point in having this line.
> 
> Plus, some reference to the HTML5 spec is useful so people know what a web notification is.
I agree.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:44
> > + * Get iconURL for the notification from @c Ewk_Notification_Info.
> 
> "iconURL" is very implementation specific for a documentation entry. Please explain what it means and separate "icon" and "URL" (and in the "@return" line you use "Icon URL", with spaces, different order and different casing.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:48
> > + * @return chacter pointer to the Icon URL of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:49
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:58
> > + * @return chacter pointer to the title of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:59
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:68
> > + * @return chacter pointer to the body contents of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:69
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:74
> > + * Get html contents URL for the notification from @c Ewk_Notification_Info.
> 
> This is not mentioned in the w3c spec, so please explain what "html contents URL" means.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:78
> > + * @return chacter pointer to the html contents url of notification.
> 
> Typo: "chacter". You also must inform the user whether he must manually free the returned pointer or not.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:79
> > + *
> 
> This empty line can be removed.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:84
> > + * Calls a callback fucntion which is registerd, after receiving a user approval.
> 
> Typo: "fucntion" and "registerd". And it should be "user's approval".
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:90
> > +EAPI void ewk_notification_permission_callback_call(const Ewk_Notification_Info* eni, Eina_Bool permitted, const char* domain);
> 
> I don't see why this should be called by users. The callback must be called automatically when the user has responded to the permission request.

This function get a user input for the permission of notification.
When user click permit or not for the notification, this function have to be called, 
because there is no return value in the request permission by smart_callback_call("notification,requestPermission").
Will be modify description.

> 
> > Source/WebKit/efl/ewk/ewk_notification.h:93
> > + * Makes a Cache for permitted domains.
> 
> Why is "Cache" capitalized? What should it mean for the reader?
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:95
> > + * This function can be called at initializing time if a application have permited domains.
> 
> Typo: "permited". "initializing" should be "initialization".
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:96
> > + * WebCore checks a permission from the cache only when checkPermission() is called from javascript.
> 
> API users are not expected to know what "WebCore" means.
> 
> > Source/WebKit/efl/ewk/ewk_notification.h:102
> > +EAPI void ewk_notification_permitted_domains_add(const Ewk_Notification_Info* eni, char** domains, int size);
> 
> The description above contradicts the code's use: this is currently the only way to add permission for a given security origin and thus cannot be called "at initializing time".

This function is for apps that is use WebKit-EFL APIs.
If the apps have permissions for the notification already, they have to call this function when it is initialized.
Will be add a detail description to ewk_notificaiton.h

> 
> > Source/WebKit/efl/ewk/ewk_notification_private.h:43
> > +void ewk_notificaiton_destroy_all(Ewk_Notification* ewkNotification);
> 
> Typo: "notificaiton"
> 
> > Source/WebKit/efl/ewk/ewk_private.h:-230
> > -
> 
> Unrelated change.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:71
> > +#include "NotificationController.h"
> 
> Is this include really needed?
> 
> > Tools/ChangeLog:9
> > +        And add "WebCore/notifications" source path to build DumpRenderTree.
> 
> No need to say this  if hereyou repeat it below.

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



More information about the webkit-unassigned mailing list