[webkit-reviews] review granted: [Bug 22722] Implement ServiceWorkerRegistration.showNotification() : [Attachment 451798] EWS v7

Sat Feb 12 19:43:57 PST 2022

Chris Dumez <cdumez at apple.com> has granted  review:
Bug 22722: Implement ServiceWorkerRegistration.showNotification()
https://bugs.webkit.org/show_bug.cgi?id=22722

Attachment 451798: EWS v7

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

--- Comment #13 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 451798
  --> https://bugs.webkit.org/attachment.cgi?id=451798
EWS v7

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

r=me but please add change logs :)

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:2
> +<script src="/resources/js-test-pre.js"></script>

Since you're not including js-test-post.js, seems you should be including
js-test.js, not js-test-pre.js.

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:10
> +	   testRunner.notifyDone();

We're supposed to call finishJSTest() with js-test instead of notifyDone().

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:14
> +    testRunner.waitUntilDone();

Can use `jsTestIsAsync = true;` since this is a js-test.

> LayoutTests/http/tests/workers/service/shownotification-allowed.html:69
> +	       testRunner.notifyDone();

ditto about finishJSTest()

> LayoutTests/http/tests/workers/service/shownotification-denied-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".

See how we're missing the "TEST COMPLETE" at the end.

> LayoutTests/http/tests/workers/service/shownotification-denied.html:2
> +<script src="/resources/js-test-pre.js"></script>

Same comments apply to this test too.

> Source/WebCore/Modules/notifications/Notification.cpp:193
> +	   auto* context = scriptExecutionContext();

I am not convinced that calling scriptExecutionContext() on the main thread
here is really safe given that this is a service worker object.

> Source/WebCore/Modules/notifications/Notification.cpp:198
> +	  
static_cast<ServiceWorkerGlobalScope*>(context)->postTaskToFireNotificationEven
t(NotificationEventType::Click, *this, { });

should be a downcast<ServiceWorkerGlobalScope>(context). Then you don't need
your RELEASE_ASSERT() above.

> Source/WebCore/Modules/notifications/Notification.cpp:217
> +	  
static_cast<ServiceWorkerGlobalScope*>(context)->postTaskToFireNotificationEven
t(NotificationEventType::Close, *this, { });

ditto.

> Source/WebCore/Modules/notifications/NotificationEvent.cpp:37
> +    auto* notification = init.notification.get();

Given that it is required and not nullable in the IDL, I think it would be good
to assert that it is not null here and pass a C++ reference to the constructor
to make it clear it is non-null. Sadly, we need to keep using a RefPtr<>
(instead of a Ref<>) in the struct because of bindings generator limitations.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:78
> +    });

Not convinced this needed to be on the next line :)

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:235
> +	       auto event = NotificationEvent::create(eventName,
protectedNotification.ptr(), action, ExtendableEvent::IsTrusted::Yes);

Ideally we'd pass a C++ reference to create() and drop the `.ptr()` here.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:236
> +	      
static_cast<ServiceWorkerGlobalScope&>(scope).dispatchEvent(event);

downcast<ServiceWorkerGlobalScope>(scope)

> Source/WebCore/workers/service/ServiceWorkerRegistration.cpp:301
> +    promise.reject(Exception { TypeError,
"ServiceWorkerRegistration.getNotifications not yet implemented"_s });

NotSupportedError may be more appropriate than TypeError here I think.

> Source/WebCore/workers/service/server/SWServer.cpp:1232
> +	   LOG(Push, "ServiceWorker import is complete, can handle push
messasge now");

Typo: messasge

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2257
> +	   LOG(Notifications, "NetworkProcess getting pending push messages for
session ID %llu", sessionID.toUInt64());

Aren't we supposed to use PRIu64 to print uint64s to be nice to our Linux
ports?

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:36
> +    static ServiceWorkerNotificationHandler& handler = *new
ServiceWorkerNotificationHandler;

Seems like what we usually use NeverDestroyed<> for?

Also, we probably want an ASSERT(isMainRunLoop()); in here since this is not
thread safe.

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:40
> +ServiceWorkerNotificationHandler::ServiceWorkerNotificationHandler()

= default; ?

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:64
> +    m_notificationToSessionMap.set(data.notificationID, data.sourceSession);

Can the key already exist in the map? If not, add() would be more efficient.

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.h:35
> +class ServiceWorkerNotificationHandler : public
NotificationManagerMessageHandler {

Class could be marked as final.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:54
> +    static WebNotificationManagerProxy* sharedManager = new
WebNotificationManagerProxy(nullptr);

Could use NeverDestroyed<>

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:135
>      globalNotificationIDs.reserveCapacity(m_globalNotificationMap.size());

reserveInitialCapacity() would be more efficient here since this is a brand new
Vector.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:261
> +   
WebProcessPool::sendToAllRemoteWorkerProcesses(Messages::WebNotificationManager
::DidUpdateNotificationDecision(origin->securityOrigin().toString(), allowed));

Don't you want to only send to ServiceWorker processes? Why send this to Shared
Worker processes too?

> Source/WebKit/UIProcess/WebProcessPool.h:838
> +void WebProcessPool::sendToAllRemoteWorkerProcesses(const T& message)

As mentioned above, maybe this should be named
sendToAllServiceWorkerProcesses().

> Source/WebKit/UIProcess/WebProcessPool.h:841
> +	   if (process.canSendMessage())

And then you could add a `&& process.isRunningServiceWorkers()` check here.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2071
> +    notifications.append(notificationID);

There has got to be a way to initialize the Vector with this notificationID
right away, maybe:
Vector<UUID> notification = { notificationID };

Trying to leverage `Vector(std::initializer_list<T> initializerList)`.

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:128
> +    if (!page && !notification.scriptExecutionContext())

I am not sure it is safe to call notification.scriptExecutionContext() on the
main thread here if we're on the main thread and this is a Notification from a
worker thread.

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:161
> +    m_notificationIDMap.set(notification.identifier(), &notification);

Can the key be already present in the map (and we need to overwrite)? If not,
add() would be more efficient.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:57
> +    callOnMainRunLoopAndWait([&result, protectedNotification = Ref {
notification }]() {

Why do we need to wait here? (I'm sure there is a good reason, I'm just
curious).

If you do need to wait, not that you could simplify the code by dropping the
`if (isMainRunLoop())` check above and only do this callOnMainRunLoopAndWait()
call. callOnMainRunLoopAndWait() already calls the lambda synchronously if
already on the main run loop.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:70
> +    callOnMainRunLoopAndWait([protectedNotification = Ref { notification
}]() {

Same comments as function above.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:85
> +    callOnMainRunLoopAndWait([&notification]() {

Same comments as function above.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:92
> +    callOnMainRunLoop([this] {

Does it need to be asynchronous always? If not, we could call
ensureOnMainRunLoop() to call synchronously when already on the main thread.

> Source/WebKit/WebProcess/WebCoreSupport/WebNotificationClient.cpp:121
> +    callOnMainRunLoopAndWait([&resultPermission, origin =
origin->data().toString()] {

Even though it is OK right now due to the implementation of toString(), I think
it would be safer / more robust to call isolatedCopy() here. isolatedCopy() is
anyway optimized when called on a rvalue reference and the ref count is 1.

> Tools/WebKitTestRunner/WebNotificationProvider.cpp:67
> +    m_permissions = adoptWK(WKMutableDictionaryCreate());

Should be in the initializer list.

> Tools/WebKitTestRunner/WebNotificationProvider.cpp:94
> +	   auto context = WKPageGetContext(page);

We probably don't need this local variable and the curly brackets.