Bug 231786 – WebKit Managed Notifications: Skeleton NotificationProvider

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

To change how Notifications work to be WebKit managed (instead of client managed) we'll add a different API::NotificationProvider to perform all the logic.

Let's land a skeleton first.

<rdar://84275562>

[Brady Eidson]

Created attachment 441326 [details]
Patch v1

[Alex Christensen]

Comment on attachment 441326 [details]
Patch v1

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

> Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.h:53
> +    bool isClientReplaceable() const final { return false; }

I don't think I understand why this is needed.  Can we go without?

> Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.mm:39
> +        return std::make_unique<WebNotificationProviderCocoa>();

makeUnique which also does a fastMalloc check.

> Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.mm:40
> +    return nullptr;

It would be nice if this returned makeUniqueRef<API::NotificationProvider>(); if it's not enabled.  Then we would never need to null check m_provider in WebNotificationManagerProxy, which can also be changed from a std::unique_ptr to a UniqueRef.  Right now it can never be null, and I think we should keep that.

[Brady Eidson]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 441326 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441326&action=review
> 
> > Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.h:53
> > +    bool isClientReplaceable() const final { return false; }
> 
> I don't think I understand why this is needed.  Can we go without?

I will explain in the ChangeLog as well, but for the record:

Without this (temporary) mechanism, you'd be unable to run existing Safari against new WebKit to get the new feature, because Safari would override the NotificationProvider through an SPI call.

> 
> > Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.mm:39
> > +        return std::make_unique<WebNotificationProviderCocoa>();
> 
> makeUnique which also does a fastMalloc check.

Yessssss

> 
> > Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.mm:40
> > +    return nullptr;
> 
> It would be nice if this returned
> makeUniqueRef<API::NotificationProvider>(); if it's not enabled.  Then we
> would never need to null check m_provider in WebNotificationManagerProxy,
> which can also be changed from a std::unique_ptr to a UniqueRef.  Right now
> it can never be null, and I think we should keep that.

This is to support the above - Note that *if* this returns null, then the WebNotificationManagerProxy will create the default API::NotificationProvider.

This is necessary for the weird branch of having this feature enabled at runtime or not.

[Alex Christensen]

Comment on attachment 441326 [details]
Patch v1

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

>>> Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.mm:40
>>> +    return nullptr;
>> 
>> It would be nice if this returned makeUniqueRef<API::NotificationProvider>(); if it's not enabled.  Then we would never need to null check m_provider in WebNotificationManagerProxy, which can also be changed from a std::unique_ptr to a UniqueRef.  Right now it can never be null, and I think we should keep that.
> 
> This is to support the above - Note that *if* this returns null, then the WebNotificationManagerProxy will create the default API::NotificationProvider.
> 
> This is necessary for the weird branch of having this feature enabled at runtime or not.

This has one call site.  Immediately after it you have this:
if (!m_provider)
    m_provider = makeUnique<API::NotificationProvider>();
I don't understand how returning null here and returning makeUnique<API::NotificationProvider>(); are different

[Alex Christensen]

Comment on attachment 441326 [details]
Patch v1

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

>>> Source/WebKit/UIProcess/Notifications/Cocoa/WebNotificationProviderCocoa.mm:40
>>> +    return nullptr;
>> 
>> It would be nice if this returned makeUniqueRef<API::NotificationProvider>(); if it's not enabled.  Then we would never need to null check m_provider in WebNotificationManagerProxy, which can also be changed from a std::unique_ptr to a UniqueRef.  Right now it can never be null, and I think we should keep that.
> 
> This is to support the above - Note that *if* this returns null, then the WebNotificationManagerProxy will create the default API::NotificationProvider.
> 
> This is necessary for the weird branch of having this feature enabled at runtime or not.

This has one call site.  Immediately after it you have this:
if (!m_provider)
    m_provider = makeUnique<API::NotificationProvider>();
I don't understand how returning null here and returning makeUnique<API::NotificationProvider>(); are different

[Brady Eidson]

Created attachment 441328 [details]
PFL v1

[Brady Eidson]

Created attachment 441339 [details]
PFL v2

[Brady Eidson]

Created attachment 441345 [details]
PFL v3

[EWS]

Committed r284240 (243049@main): <https://commits.webkit.org/243049@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441345 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 231926

[Brady Eidson]

Created attachment 441736 [details]
New approach v1

[Brady Eidson]

Created attachment 441738 [details]
New approach v2

[Alex Christensen]

Comment on attachment 441736 [details]
New approach v1

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:295
> +    if (decoder.messageName() == Messages::WebPageProxy::ShowNotification::name())

This is usually in generated code, and WebPageProxies aren't in the network process.  I would add this:
// FIXME: move these to a new message receiver type.
That way we can make the NetworkSession own one of the new type and the WebPageProxy also own one of the new type, and both would forward messages to that new type and this would be auto-generated.

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:141
> +template<typename U> bool sendNotificationMessage(WebProcess& process, U&& message, WebPage* page)

Can this be a WebPage& ?

> Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:148
> +    {

unnecessary

[Brady Eidson]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 441736 [details]
> New approach v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441736&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:295
> > +    if (decoder.messageName() == Messages::WebPageProxy::ShowNotification::name())
> 
> This is usually in generated code, and WebPageProxies aren't in the network
> process.  I would add this:
> // FIXME: move these to a new message receiver type.
> That way we can make the NetworkSession own one of the new type and the
> WebPageProxy also own one of the new type, and both would forward messages
> to that new type and this would be auto-generated.

Yes, we can autogenerate that type of thing in the future, but manual code will still have to be written for both on both sides of the wire.

There's no example of this yet. Yay for new ground.

> 
> > Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:141
> > +template<typename U> bool sendNotificationMessage(WebProcess& process, U&& message, WebPage* page)
> 
> Can this be a WebPage& ?
> 
> > Source/WebKit/WebProcess/Notifications/WebNotificationManager.cpp:148
> > +    {
> 
> unnecessary

Fixing these.

[Brady Eidson]

Created attachment 441746 [details]
New approach v3

[Chris Dumez]

Comment on attachment 441736 [details]
New approach v1

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:274
> +    if (decoder.messageReceiverName() == Messages::WebPageProxy::messageReceiverName()) {

This looks wrong. I don't think NetworkConnectionToWebProcess should be dealing with WebPageProxy messages. Why can't the call site choose an appropriate message name based on which connection it is sending the IPC on?

If this isn't possible, an alternative would be to introduce a new MessageReceiver type (e.g. WebNotificationProxy) in Shared/ that could be reused from both the network process and the UIProcess. It would look a lot nicer than having the network process handle WebPageProxy messages (which is super confusing).

[Chris Dumez]

Comment on attachment 441746 [details]
New approach v3

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

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:274
> +    if (decoder.messageReceiverName() == Messages::WebPageProxy::messageReceiverName()) {

Looks like this new approach has the same issue.

[Alex Christensen]

Comment on attachment 441746 [details]
New approach v3

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

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.h:41
> +    void cancelNotification(uint64_t notificationID);

This should be a strongly typed identifier.

[Brady Eidson]

(In reply to Chris Dumez from comment #16)
> Comment on attachment 441736 [details]
> New approach v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441736&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:274
> > +    if (decoder.messageReceiverName() == Messages::WebPageProxy::messageReceiverName()) {
> 
> This looks wrong. I don't think NetworkConnectionToWebProcess should be
> dealing with WebPageProxy messages. Why can't the call site choose an
> appropriate message name based on which connection it is sending the IPC on?

I *literally asked you about this yesterday on Slack* and you said "No reason why not"


Why should we duplicate message types for the exact identically same message (we shouldn't)

> If this isn't possible, an alternative would be to introduce a new
> MessageReceiver type (e.g. WebNotificationProxy) in Shared/ that could be
> reused from both the network process and the UIProcess. It would look a lot
> nicer than having the network process handle WebPageProxy messages (which is
> super confusing).

That's exactly what the FIXME says.

[Brady Eidson]

(In reply to Alex Christensen from comment #18)
> Comment on attachment 441746 [details]
> New approach v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441746&action=review
> 
> > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.h:41
> > +    void cancelNotification(uint64_t notificationID);
> 
> This should be a strongly typed identifier.

Seems like relevant cleanup for later (definitely not relevant to this patch)

[Brady Eidson]

I'll work on the message receiver bit.

Sad about the misunderstanding yesterday that led me to skip bothering with it.

[Brady Eidson]

That was done in https://trac.webkit.org/changeset/284564/webkit

Rebasing this patch on that work now.

[Brady Eidson]

Created attachment 441955 [details]
Patch with better messaging v1

[Alex Christensen]

Comment on attachment 441955 [details]
Patch with better messaging v1

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

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

remove

[Alex Christensen]

Comment on attachment 441955 [details]
Patch with better messaging v1

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

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.h:2
> + * Copyright (C) 2020 Apple Inc. All rights reserved.

2021

[Brady Eidson]

Created attachment 441959 [details]
PFL v1

[Brady Eidson]

Created attachment 441969 [details]
PFL v2

[Brady Eidson]

Created attachment 441971 [details]
Another try PFL

[Brady Eidson]

Created attachment 441973 [details]
PFL again

[Brady Eidson]

Have to disable BuiltInNotificationsEnabled on some layouttests for now.

Re-enabling is covered by <rdar://84492078>

[Brady Eidson]

Created attachment 441982 [details]
PFL maybe the last one?

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Brady Eidson]

Created attachment 441984 [details]
PFL with better way to disable these tests

[EWS]

Committed r284591 (243324@main): <https://commits.webkit.org/243324@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 441984 [details].