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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 3 09:23:07 PDT 2011


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


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


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:


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.

More information about the webkit-unassigned mailing list