[webkit-reviews] review denied: [Bug 73572] [WK2] Add further support for notifications : [Attachment 117495] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jon Lee <jonlee at apple.com>'s request
for review:
Bug 73572: [WK2] Add further support for notifications
https://bugs.webkit.org/show_bug.cgi?id=73572

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=117495&action=review


> Source/WebCore/notifications/Notification.idl:45
> +	   attribute EventListener onshow;
> +	   // TODO: remove this event listener
>	   attribute EventListener ondisplay;

I don’t like this comment. First, we don’t use TODO. Second, we normally use
sentence capitalization. Third, this is between two attributes and I don’t know
which it refers to. Fourth, this gives no rationale. Why should this be done,
and why wasn’t it already done?

> Source/WebKit2/UIProcess/API/C/WKNotification.h:39
> +WK_EXPORT uint64_t WKNotificationGetNotificationID(WKNotificationRef
notification);

Can this be just named GetID instead of GetNotificationID?

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:76
>  void WebNotificationManagerProxy::show(const String& title, const String&
body, uint64_t notificationID)
>  {
> -    RefPtr<WebNotification> notification = WebNotification::create(title,
body);
> +    m_provider.addNotificationManager(this);
> +
> +    RefPtr<WebNotification> notification = WebNotification::create(title,
body, notificationID);
> +    m_notifications.set(notificationID, notification);
>      m_provider.show(notification.get());
>  }

You should probably check for invalid notification IDs, since the hash table
code will malfunction spectacularly if you try to use a 0 or -1.

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:85
> +    if (!m_notifications.contains(notificationID))
> +	   return;
> +    
> +    m_provider.addNotificationManager(this);
> +    WebNotification* notification =
m_notifications.get(notificationID).get();
> +    m_provider.cancel(notification);

It’s more efficient to do just a get or a find, not a combination of both a
contains and a get. Since get will return 0 I think you can just use get.

You should probably check for invalid notification IDs, since the hash table
code will malfunction spectacularly if you try to use a 0 or -1.

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:92
> +void WebNotificationManagerProxy::didDestroyNotification(uint64_t
notificationID)
> +{
> +    RefPtr<WebNotification> notification =
m_notifications.take(notificationID);
> +    m_provider.didDestroyNotification(notification.get());
> +}

You should probably check for invalid notification IDs, since the hash table
code will malfunction spectacularly if you try to use a 0 or -1.

You should probably check for null to make it harmless to call this with a bad
notification ID.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:100
> +void WebNotificationManager::didShowNotification(uint64_t notificationID)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    if (!m_notificationIDMap.contains(notificationID))
> +	   return;
> +    m_notificationIDMap.get(notificationID)->dispatchShowEvent();
> +#endif
> +}

You should probably check for invalid notification IDs, since the hash table
code will malfunction spectacularly if you try to use a 0 or -1.

It’s bad for performance to do both a contains and a get. You should just do
the get and check for 0.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:109
> +void WebNotificationManager::didClickNotification(uint64_t notificationID)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    if (!m_notificationIDMap.contains(notificationID))
> +	   return;
> +    m_notificationIDMap.get(notificationID)->dispatchClickEvent();
> +#endif
> +}

You should probably check for invalid notification IDs, since the hash table
code will malfunction spectacularly if you try to use a 0 or -1.

It’s bad for performance to do both a contains and a get. You should just do
the get and check for 0.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:122
> +void WebNotificationManager::didCloseNotifications(const Vector<uint64_t>&
notificationIDs)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    size_t count = notificationIDs.size();
> +    for (size_t i = 0; i < count; ++i) {
> +	   uint64_t notificationID = notificationIDs[i];
> +	   if (!m_notificationIDMap.contains(notificationID))
> +	       return;
> +	   m_notificationIDMap.get(notificationID)->dispatchCloseEvent();
> +    }
>  #endif
>  }

You should probably check for invalid notification IDs, since the hash table
code will malfunction spectacularly if you try to use a 0 or -1.

It’s bad for performance to do both a contains and a get. You should just do
the get and check for 0.

> Source/WebKit2/WebProcess/WebCoreSupport/WebNotificationClient.cpp:72
>      notImplemented();

Need to use UNUSED_PARAM here.