[webkit-reviews] review granted: [Bug 237983] Remove push subscriptions when associated service worker registrations are removed : [Attachment 454899] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Ben Nham <nham at apple.com>'s
request for review:
Bug 237983: Remove push subscriptions when associated service worker
registrations are removed
https://bugs.webkit.org/show_bug.cgi?id=237983

Attachment 454899: Patch

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




--- Comment #3 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 454899
  --> https://bugs.webkit.org/attachment.cgi?id=454899
Patch

LGTM.
I am wondering whether there is a chance that things get dysinchronized, say
the SQL service worker is damaged so now we loose all registrations but keep
some push subscriptions. Maybe there should be something ensuring everything is
fine.
For instance, if we push a message and there is no corresponding service worker
registration, we should probably remove the corresponding push subscription.

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2335
> +	  
session->notificationManager().getPushSubscription(WTFMove(scopeURL), [callback
= WTFMove(callback)](auto &&result) mutable {

Preexisting but I would not expect getPushSubscription to require a URL&&.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2341
> +	       callback(true);

Could be written as callback(result && !!result.value()) ?

> Source/WebKit/webpushd/PushService.mm:394
> +		       RELEASE_LOG(Push,
"PushSubscription.unsubscribe(bundleID: %{public}s, scope: %{sensitive}s)
failed with domain: %{public}s code: %lld)", m_bundleIdentifier.utf8().data(),
m_scope.utf8().data(), error.domain.UTF8String ?: "none",
static_cast<int64_t>(error.code));

RELEASE_LOG_IF which will not require the RELEASE_LOG_DISABLED around the if.