| Summary: | WebKit Managed Notifications: Skeleton NotificationProvider | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||||||||||||||||
| Component: | WebKit Misc. | Assignee: | Brady Eidson <beidson> | ||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | achristensen, annulen, benjamin, cdumez, clopez, cmarcelo, commit-queue, ews-watchlist, gyuyoung.kim, hi, mkwst, ryuan.choi, sergio, youennf | ||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||
| Bug Depends on: | 231926, 231931 | ||||||||||||||||||||||||||||||||
| Bug Blocks: | 199105 | ||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||
|
Description
Brady Eidson
2021-10-14 16:58:22 PDT
Created attachment 441326 [details]
Patch v1
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. (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. 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 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 Created attachment 441328 [details]
PFL v1
Created attachment 441339 [details]
PFL v2
Created attachment 441345 [details]
PFL v3
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]. Re-opened since this is blocked by bug 231926 Created attachment 441736 [details]
New approach v1
Created attachment 441738 [details]
New approach v2
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 (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. Created attachment 441746 [details]
New approach v3
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). 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. 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. (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. (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) I'll work on the message receiver bit. Sad about the misunderstanding yesterday that led me to skip bothering with it. That was done in https://trac.webkit.org/changeset/284564/webkit Rebasing this patch on that work now. Created attachment 441955 [details]
Patch with better messaging v1
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 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 Created attachment 441959 [details]
PFL v1
Created attachment 441969 [details]
PFL v2
Created attachment 441971 [details]
Another try PFL
Created attachment 441973 [details]
PFL again
Have to disable BuiltInNotificationsEnabled on some layouttests for now. Re-enabling is covered by <rdar://84492078> Created attachment 441982 [details]
PFL maybe the last one?
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 Created attachment 441984 [details]
PFL with better way to disable these tests
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]. |