[webkit-reviews] review granted: [Bug 238068] Remove push subscriptions when permissions are reset : [Attachment 455778] patch feedback

[bugzilla-daemon at webkit.org]

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

Attachment 455778: patch feedback

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




--- Comment #13 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 455778
  --> https://bugs.webkit.org/attachment.cgi?id=455778
patch feedback

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

> Source/WebKit/ChangeLog:20
> +	   back on. We want the subscription object to stay the same after the
preference is toggled

I am not 100% sure about this policy but I can live with it.
As a user, I sometimes change back from Allow to Prompt some permissions.
I would not expect Prompt to remove the subscriptions while Denied would keep
them.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1779
> +	   permission = it->value ? PushPermissionState::Granted :
PushPermissionState::Denied;

In case permission is denied, I would exit early, it does not seem worth to
start the network process proxy for no good reason.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:278
> +	   if (!networkProcess && dataStore.isPersistent()) {

It is a bit weird to do this only for the first persistent session, can you
explain?
I would be tempted to do this for all session IDs.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:288
> +	   networkProcess->deletePushAndNotificationRegistration(sessionID,
origin, [originString = origin.toString()](auto&&) { });

Should we add a deletePushAndNotificationRegistration taking a vector of
origins to remove the multiple IPC dance?
Also, why do we need originString in the lambda?

> Source/WebKit/webpushd/WebPushDaemon.mm:502
> +   
connection->enqueueAppBundleRequest(makeUnique<AppBundleDeletionRequest>(*conne
ction, originString, [this, originString = String { originString }, replySender
= WTFMove(replySender), bundleIdentifier =
connection->hostAppCodeSigningIdentifier()](const String& result) mutable {

s/const String&/auto/