[webkit-reviews] review granted: [Bug 240273] Make sure calling showNotification will extend the service worker lifetime : [Attachment 459145] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 240273: Make sure calling showNotification will extend the service worker
lifetime
https://bugs.webkit.org/show_bug.cgi?id=240273

Attachment 459145: Patch

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




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 459145
  --> https://bugs.webkit.org/attachment.cgi?id=459145
Patch

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

r=me with comments

> Source/WebCore/Modules/notifications/Notification.cpp:149
> +    auto scope = makeScopeExit([&callback] { callback(); });

I feel like using a CompletionHandlerCallingScope would be much nicer, since
you could WTFMove() here.

> Source/WebCore/Modules/notifications/Notification.cpp:176
> +    scope.release();

And the release() here would give you your completion handler again so you can
capture it in your lambda. It is less error prone.

> Source/WebCore/dom/ScriptExecutionContext.cpp:769
> +CompletionHandler<void()>
ScriptExecutionContext::notificationCallback(NotificationCallbackIdentifier
identifier)

Should have "take" in the name since it is actually modifying the underlying
map.

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:84
> +    m_pushEvent = &pushEvent;

Are we sure m_pushEvent was not already initialized to a pushEvent here?

> Source/WebCore/workers/service/ServiceWorkerGlobalScope.cpp:86
> +    m_pushEvent = nullptr;

If so, we'd lose that previous pushEvent here. Sometimes we want a "stack"
behavior for this sort of things and restore the previous value of m_pushEvent
rather than clearing it. I don't know if it applies here but if it does, we
should consider using SetForScope instead.

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:106
> +    callback();

Is it really OK here to be calling the callback without we've received the
response to the IPC below? It looks weird.

> Source/WebKit/UIProcess/Notifications/ServiceWorkerNotificationHandler.cpp:59
> +	   callback();

Given how callback is used, I'd recommend a ScopeExit or a
CompletionHandlerCallingScope.

> Source/WebKitLegacy/mac/WebCoreSupport/WebNotificationClient.mm:71
> +    callback();

You are failing to call callback() in the early return case above. I'd suggest
using a ScopeExit or a CompletionHanderCallingScope to avoid such bugs.

> Tools/TestWebKitAPI/TestNotificationProvider.cpp:142
> +bool TestNotificationProvider::hasReceivedNotification() const

I'd suggest inlining those 2 in the header given that those are trivial getter
/ setters.