[webkit-reviews] review denied: [Bug 73544] [EFL] Add Web Notification feature for WebKit-EFL : [Attachment 121414] Fixed for the all of comments(#25)

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

Raphael Kubo da Costa <kubo at profusion.mobi> has denied Kihong Kwon
<kihong.kwon at samsung.com>'s request for review:
Bug 73544: [EFL] Add Web Notification feature for WebKit-EFL
https://bugs.webkit.org/show_bug.cgi?id=73544

Attachment 121414: Fixed for the all of comments(#25)
https://bugs.webkit.org/attachment.cgi?id=121414&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
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.strin
g().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.

More information about the webkit-reviews mailing list