[webkit-reviews] review granted: [Bug 116428] [WK2] Notifications clobber each other with multiple processes : [Attachment 202943] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jon Lee <jonlee at apple.com>'s request
for review:
Bug 116428: [WK2] Notifications clobber each other with multiple processes
https://bugs.webkit.org/show_bug.cgi?id=116428

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

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


I am not sure I understand why so many different IDs are needed and the data
structure so indirect. For example, why can’t we use a WebPageProxy* in the
hash table key rather than a page ID? And why not have this be a
per-WebPageProxy data structure that just uses the notification ID as the key
and has the WebNotification object as the value. If we don’t want to store it
in a WebPageProxy data member, we could still use a hash table in the
notification manager proxy, but the key should just be a WebPageProxy and the
values could be OwnPtr<HashMap<uint64_t, RefPtr<WebNotification>>>.

If I was doing this, maybe I would have a per-WePageProxy data structure, or
maybe a per-WebProcessProxy one, that maps notification IDs from that process
to WebNotification objects. I don’t think we need a global notification ID.

I’m going to say review+ but I think we can simplify this in the future.

> Source/WebKit2/ChangeLog:12
> +	   With multiple processes, the notification IDs, when passed up to the
UI process, can clobber
> +	   each other. To fix this, we need to maintain a global map of
notification IDs. This map is
> +	   keyed by its own unique notification ID, and maps to a pair
containing the web page ID and that
> +	   web page's ID for the notification.

I don’t understand how it follows that we need a global map of notification
IDs. Could WebNotificationManagerProxy instead be made into something that’s
per-process?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:106
> -    if (!isNotificationIDValid(notificationID))
> +    if (!isNotificationIDValid(pageNotificationID))
>	   return;

This check is important if the notification ID itself is used as a hash key and
therefore we have to ensure that it’s neither 0 nor -1. But in the current
patch, since the key involves both the page and the ID, this check is not
needed nor do I think it’s helpful.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:110
> +    pair<uint64_t, uint64_t> notificationPairID =
make_pair(webPage->pageID(), pageNotificationID);

This is a “notification ID pair”, not a “notification pair ID”.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:119
> -    if (!isNotificationIDValid(notificationID))
> +    if (!isNotificationIDValid(pageNotificationID))
>	   return;

This check was important before when the notification ID itself was a hash key
and therefore we had to ensure that it’s neither 0 nor -1. But now since the
key involves both the page and the ID, there’s no need for this check.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:121
> +    const auto& notificationItem =
m_notifications.find(make_pair(webPage->pageID(), pageNotificationID));

Since an iterator is a scalar, this can just be "auto" rather than "const
auto&".

But why use get here instead of find? With get we would get a pair with zero
for both, and so the notification would be 0. I think using find makes the code
harder to read.

I’d write:

    if (WebNotification* notification =
m_notifications.get(make_pair(webPage->pageID(),
pageNotificationID)).second.get())
	m_provider.cancel(notification);

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:134
> +    const auto& notificationItem =
m_notifications.find(make_pair(webPage->pageID(), pageNotificationID));

Same comment about auto vs const auto&.

But also, again why not use get?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:158
> +void WebNotificationManagerProxy::clearNotifications(WebPageProxy* webPage)
> +{
> +    clearNotifications(webPage, Vector<uint64_t>(), pageIDsMatch);
> +}

This seems unnecessarily inefficient. We have to iterate the entire
m_notifications data structure because we chose to key it only off pairs. Why
not group all the notifications for a page in a page-specific hash map so we
don’t have to iterate everything?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:163
> +void WebNotificationManagerProxy::clearNotifications(WebPageProxy* webPage,
const Vector<uint64_t>& pageNotificationIDs)
> +{
> +    clearNotifications(webPage, pageNotificationIDs,
pageAndNotificationIDsMatch);
> +}

This seems like a strange idiom. We have an indexed data structure with webPage
and notification ID as keys. But instead of using that indexing, we iterate the
whole thing. If we’re going to iterate the whole thing, why not use a Vector
instead of a HashMap?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:183
> +	   const pair<uint64_t, uint64_t>& pageNotification =
m_globalNotificationMap.take(*it);

There’s no need to say const pair<uint64_t, uint64_t>& here. The const& doesn’t
help, and auto would work.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:192
> +    const auto& it = m_globalNotificationMap.find(globalNotificationID);

Just auto would be OK here, no need for const auto&.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:207
> +    const auto& it = m_globalNotificationMap.find(globalNotificationID);

auto would be fine here, no need for const auto&.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.h:86
> +    // Pair comprised of web page ID and the web process's notification ID
> +    HashMap<uint64_t, pair<uint64_t, uint64_t>> m_globalNotificationMap;
> +    // Key pair comprised of web page ID and the web process's notification
ID; value pair comprised of global notification ID, and notification object
> +    HashMap<pair<uint64_t, uint64_t>, pair<uint64_t,
RefPtr<WebNotification>>> m_notifications;

These us of "pair" makes this code hard to read. You have to think about what
the first and what the second item is. I think I might prefer to use a struct
instead so the members could have meaningful names.