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

[bugzilla-daemon at webkit.org]

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





--- Comment #46 from Gustavo Noronha (kov) <gns at gnome.org>  2011-10-03 09:23:06 PST ---
(From update of attachment 108810)
View in context: https://bugs.webkit.org/attachment.cgi?id=108810&action=review

Other than that name (replaces), the patch looks fine to me. I was feeling a need to bikeshed on the signal names (suggest they follow our usual naming scheme that uses past tenses), but I managed to control myself and think they're fine as they are.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:163
> + * webkit_web_notification_get_icon_uri:

s/_uri//

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:255
> +gboolean webkit_web_notification_replaces(WebKitWebNotification* self, WebKitWebNotification* other)

I think this name is misleading. Look at it this way:

self.replaces(other)

It would lead me to believe that we are checking if self replaces other; I don't think it's all the same, is it? How about:

self.is_replaced_by(other)?

So webkit_web_notification_is_replaced_by(self, other);

>> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:286
>> +            || (coreNotification1 == coreNotification2)
> 
> As I was asking in the previous review, is this superfluous?

s/superfluous/redundant/ =D seems to be, indeed.

> Source/WebKit/gtk/webkit/webkitwebnotification.cpp:313
> +// Can't use the standard 'kit' name as it clashes with the one defined in WebKitDomNotification
> +WebKitWebNotification* kitNew(WebCore::Notification* coreNotification)

We changed all kit()s that created new instances to kitNew anyway, didn't we? So the name looks good.

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