[webkit-reviews] review denied: [Bug 61140] [GTK] Add HTML5 Notifications support : [Attachment 104354] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Alexandre Mazari
<scaroo at gmail.com>'s request for review:
Bug 61140: [GTK] Add HTML5 Notifications support
https://bugs.webkit.org/show_bug.cgi?id=61140

Attachment 104354: Patch
https://bugs.webkit.org/attachment.cgi?id=104354&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=104354&action=review


> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:777
> +NotificationPresenter* ChromeClient::notificationPresenter() const

If this method creates a Presenter it should be named something like
createNotificationPresenter() and probably pass its reference to the caller, I
think, like createPopupMenu() does for instance.

> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:31
> +#if ENABLE(NOTIFICATIONS)

This could just go before the includes

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:79
> +	   g_error ("%s", error->message);
> +	   g_error_free (error);

No space before ( please :)

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:88
> + * notification. Internally, it propagate an "error" event in the DOM

s/propagate/propagates

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:103
> + * notification. Internally, it propagate a "display" event in the DOM

Ditto

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:118
> + * been clicked. Internally, it propagate a "click" event in the DOM

Ditto

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:177
> +    static HashMap<KURL, GdkPixbuf* > pixbuf_cache;
> +
> +    KURL icon_uri = core(WEBKIT_DOM_NOTIFICATION(self))->iconURL();

These variables are unused it seems.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:254
> + * Returns: whether @self replaces (as defined in the Notifications specs)

Replace what?

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:255
> + * other.

other... notification I guess!

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:290
> +    return ((notification == notification2)
> +	       || (coreNotification1 == coreNotification2)

Hum is it really necessary to compare both the WebKitWebNotifications and
Notifications? I feel like one comparison would be enough here.

> Source/WebKit/gtk/webkit/webkitwebnotificationprivate.h:4
> + * webkit-web-notification.h - Header for WebKitWebNotification
> + *
> + * Copyright (C) 2010 Igalia S.L.

header name and copyright year are wrong

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2769
> +	* For each notification, one of WebKitNotification:displayed or
> +	* WebKitNotification:failed *MUST* be called at actual display-time so

Missing : ?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2795
> +	* remove it from the display; if it has not yet be displayed,

s/be/been

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2796
> +	* the user agent must prevent its being displayed.

"must prevent its being displayed" is odd, do you mean "must prevent its
display" ?