[Webkit-unassigned] [Bug 61140] [GTK] Add HTML5 Notifications support

[bugzilla-daemon at webkit.org]

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





--- Comment #50 from Alexandre Mazari <scaroo at gmail.com>  2011-11-03 08:11:12 PST ---
Hi pnormand,

Thanks for your review.

(In reply to comment #49)
> (From update of attachment 112683 [details])
> 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?
>

Sure.

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

Indeed, it is only used in DRT for now but can also be useful in client code (for test/logging purpose). Generally I think it is a good idea to have a default textual representation of an object.
Anyway, I'll move it in the testing code.  

> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
> > +            || (coreNotification1 == coreNotification2)
> 
> Can you remove this? As asked in the previous 2 reviews....
> 

Well, i dont think the comparison are redundant here : the first one check for reference equality of the Gobject wrapper, the second one for the reference ('=' aint overloaded for Notification class). It is here for performance reason, avoiding checking for fields equality if the refs match. A same core Notification might be wrapped by several GObjects even if we try to avoid such a situation.
Thus I am not inclined on removing that check.

> >> Source/WebKit/gtk/webkit/webkitwebview.h:85
> >> +{
> > 
> > This { should be at the end of the previous line  [whitespace/braces] [4]
> 
> Please fix this

False positive, I followed surrounding code format.

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