[webkit-reviews] review denied: [Bug 61140] [GTK] Add HTML5 Notifications support : [Attachment 112683] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Nov 3 07:53:40 PDT 2011
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 112683: Patch
https://bugs.webkit.org/attachment.cgi?id=112683&action=review
------- Additional Comments from Philippe Normand <pnormand at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112683&action=review
> Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.h:31
> +#if ENABLE(NOTIFICATIONS)
Can you move the #if before the includes as I suggested in the previous two
reviews?
> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:275
> +char* webkit_web_notification_to_string (WebKitWebNotification*
notification)
> +{
> + return webkit_web_notification_is_html(notification) ?
> + g_strdup_printf("DESKTOP NOTIFICATION: contents at %s",
webkit_web_notification_get_uri(notification)) :
> + g_strdup_printf("DESKTOP NOTIFICATION:%s icon %s, title %s, text %s",
> + webkit_web_notification_get_direction(notification) ==
GTK_TEXT_DIR_RTL ? "(RTL)" : "",
> + webkit_web_notification_get_icon_uri(notification),
> + webkit_web_notification_get_title(notification),
> + webkit_web_notification_get_body(notification));
> +}
AFAIK this is used only in DRT and quite specific to the tests. Please move
this code to DRT, no need to be a function either.
> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
> + || (coreNotification1 == coreNotification2)
Can you remove this? As asked in the previous 2 reviews....
>> Source/WebKit/gtk/webkit/webkitwebview.h:85
>> +{
>
> This { should be at the end of the previous line [whitespace/braces] [4]
Please fix this
More information about the webkit-reviews
mailing list