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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jan 8 18:00:02 PST 2012 

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


Raphael Kubo da Costa <kubo at profusion.mobi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #121414|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #27 from Raphael Kubo da Costa <kubo at profusion.mobi>  2012-01-08 18:00:02 PST ---
(From update of attachment 121414)
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.
 * The queueing of callback calls seems flawed (a single callback is kept at a time).
 * "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).
 * 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.
 * 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.

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.

> 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?

> 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.

> 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.

> 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.

> 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?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.cpp:-54
> -    notImplemented();

Why? This is still not implemented.

> 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).

> 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?

> Source/WebKit/efl/WebCoreSupport/NotificationPresenterClientEfl.h:43
> +    virtual void cancelRequestsForPermission(ScriptExecutionContext*) { }

Doesn't this need to be implemented?

> 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.

> 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?

> 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.

> 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.

> 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".

> 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 here if you 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