[webkit-reviews] review granted: [Bug 238544] ServiceWorkerRegistration.getNotifications should list all persistent notifications : [Attachment 456220] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 238544: ServiceWorkerRegistration.getNotifications should list all
persistent notifications
https://bugs.webkit.org/show_bug.cgi?id=238544

Attachment 456220: Patch

https://bugs.webkit.org/attachment.cgi?id=456220&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 456220
  --> https://bugs.webkit.org/attachment.cgi?id=456220
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456220&action=review

Not confident that all this code

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:627
> +	   std::sort(data.begin(), data.end(), [](auto& a, auto& b) { return
a.creationTime < b.creationTime; });

Is it safe to use an unstable sort here? If there are any two notifications
with the same creationTime, then they could end up shuffled randomly by the
sort algorithm. If we used a stable sort, then they would stay in the order
they are already in, even if the times happened to be identical.

> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:84
> +    for (auto& callback : navigationPreloadStateCallbacks.values())
> +	   callback(Exception { AbortError, "context stopped"_s });

This is in hash table (pseudo-random) order. Is that OK?

> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:88
> +    for (auto& callback : getNotificationsCallbacks.values())
>	   callback(Exception { AbortError, "context stopped"_s });

Ditto.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:306
> +    Vector<NotificationData> result;

Should we be reserving initial capacity? Does this typically get sized the same
as m_notifications or is it usually a much smaller subset?

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:315
> +    callback(WTFMove(result));

Can this vector be long-lived? If so, maybe we should do shrinkToFit after
building it and before transferring ownership?