Created attachment 439424 [details] Proposed patch PassKit offers presentationSceneIdentifierForPaymentAuthorizationController delegate call, but WebKit made no effort to respond to that.
Comment on attachment 439424 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=439424&action=review awesome work! the core logic looks good =D tho in order to land this you'll need to add a ChangeLog entry by running `./path/to/WebKit/Tools/Scripts/prepare-ChangeLog -o` and include some info about why the change is being made as well as explaining what's being changed also FYI many of the comments below also apply to more than just that one place in your patch, so please keep that in mind :) > Source/WTF/wtf/PlatformEnableCocoa.h:90 > +#if !defined(ENABLE_APPLE_PAY_REMOTE_UI_USES_SCENE) && defined(ENABLE_APPLE_PAY_REMOTE_UI) && HAVE(UIKIT_WEBKIT_INTERNALS) I don't think we have any `ENABLE_*` flags that depend on and/or are defined by other `ENABLE_*` flags 🤔 > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:359 > + void paymentCoordinatorPresentingWindowSceneIdentifier(WebKit::WebPageProxyIdentifier, CompletionHandler<void(String)>&&) final; this (along with almost everything else in this patch) should be guarded by `ENABLE(APPLE_PAY_REMOTE_UI_USES_SCENE)` also, you dont need the `WebKit::` since we're already in the `WebKit` namespace (ditto in most places that `WebKit::` can be found in the rest of this patch) > Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.h:89 > + virtual void presentInSceneIdentifiedBy(String, CompletionHandler<void(bool)>&&) = 0; We usually try not to leave trailing prepositions, so I'd just name it `virtual void presentInScene(const String& sceneIdentifier, CompletionHandler<void(bool)>&&) = 0;`. You should also make the first argument `const String&`. When used as a return/parameter type, it should almost always be `const String&` (or `String&&` if you're moving it). (ditto in most places that `String,` or `(String` can be found in the rest of this patch) > Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.h:90 > + String sceneIdentifier() { return m_sceneIdentifier; } you can add a bunch of `const` here ``` const String& sceneIdentifier() const { return m_sceneIdentifier; } ``` > Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.h:101 > virtual WKPaymentAuthorizationDelegate *platformDelegate() = 0; > +#if PLATFORM(IOS_FAMILY) && ENABLE(APPLE_PAY_REMOTE_UI) Style: I'd add a newline between these since one is a method and the other is a member variable (and they're unrelated to eachother) > Source/WebKit/Platform/ios/PaymentAuthorizationController.mm:111 > + String sceneID = _presenter->sceneIdentifier(); > + return sceneID.isEmpty() ? nil : static_cast<NSString *>(sceneID); you can use `nsStringNilIfEmpty` to do all this in one go instead :) ``` if (!_presenter) return nil; return nsStringNilIfEmpty(_presenter->sceneIdentifier()); ``` > Source/WebKit/Platform/ios/PaymentAuthorizationController.mm:159 > + present(nullptr, WTFMove(completionHandler)); NIT: the first argument should probably be `nil` since it's an ObjC type > Source/WebKit/Shared/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:59 > + m_client.paymentCoordinatorPresentingWindowSceneIdentifier(webPageProxyID, [this, completionHandler = WTFMove(completionHandler)](String sceneID) mutable { since this involves an IPC roundtrip, you'll want to add `weakThis = makeWeakPtr(*this)` to the capture list and then check `if (!weakThis)` > Source/WebKit/Shared/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:60 > + this->m_authorizationPresenter->presentInSceneIdentifiedBy(sceneID, WTFMove(completionHandler)); you can drop the `this->` since you're capturing `this` > Source/WebKit/Shared/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:64 > +#endif you should add a `UNUSED_PARAM(webPageProxyID);` above this so that when `ENABLE(APPLE_PAY_REMOTE_UI_USES_SCENE)` is not true you wont get an error about an unused variable > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1188 > + auto sceneID = page->pageClient().sceneID(); > + completionHandler(sceneID); NIT: I'd just inline this and avoid creating a variable > Source/WebKit/UIProcess/ios/PageClientImplIOS.mm:1028 > + NSString *sceneID = [m_contentView window].windowScene._persistenceIdentifier; I think in order to get this to build in OpenSource you'll need to add a stub for it in `Source/WebKit/Platform/spi/ios/UIKitSPI.h` (and include the file above) ``` @interface UIScene () @property (nullable, nonatomic, readonly) NSString *_persistenceIdentifier; @end ``` On a related note, why this instead of just `_sceneIdentifier`?
Created attachment 439570 [details] Patch
Comment on attachment 439570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439570&action=review > Source/WebKit/ChangeLog:13 > + (WebKit::PaymentAuthorizationPresenter::sceneIdentifier const): Holds temporarily the scene ID we'd like to use when presenting the payme > +nt sheet, to be read by WKPaymentAuthorizationControllerDelegate. oops? > Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:359 > + void paymentCoordinatorPresentingWindowSceneIdentifier(WebPageProxyIdentifier, CompletionHandler<void(String)>&&) final; the `String` should probably be a `const String&` (or `String&&`) > Source/WebKit/Shared/ApplePay/WebPaymentCoordinatorProxy.h:162 > + void platformShowPaymentUI(const WebPageProxyIdentifier&, const URL& originatingURL, const Vector<URL>& linkIconURLs, const WebCore::ApplePaySessionPaymentRequest&, CompletionHandler<void(bool)>&&); I don't think `WebPageProxyIdentifier` needs/wants to be `const` or `&` > Source/WebKit/Shared/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:61 > + weakThis->m_authorizationPresenter->presentInScene(sceneID, WTFMove(completionHandler)); this should also check that `m_authorizationPresenter` is valid too ``` Ref strongThis = weakThis.get(); if (!strongThis) { completionHandler(false); return; } if (!strongThis->m_authorizationPresenter) { completionHandler(false); return; } strongThis->m_authorizationPresenter->presentInScene(sceneID, WTFMove(completionHandler)); ``` > Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in:81 > + PaymentCoordinatorPresentingWindowSceneIdentifier(WebKit::WebPageProxyIdentifier webPageProxyIdentifier) -> (String sceneID) Async NIT: I'd rename this to something more "verb-y" like `GetWindowSceneIdentifierForPaymentPresentation`
Created attachment 439632 [details] Patch
Comment on attachment 439632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=439632&action=review > Source/WebKit/Platform/ios/PaymentAuthorizationController.h:54 > + void presentInScene(const String&, CompletionHandler<void(bool)>&&) final; NIT: this should above any member variables, probably right below `present` > Source/WebKit/Shared/ApplePay/WebPaymentCoordinatorProxy.h:96 > + virtual void paymentCoordinatorPresentingWindowSceneIdentifier(WebPageProxyIdentifier, CompletionHandler<void(const String&)>&&) = 0; NIT: I'd also rename this to `getWindowSceneIdentifierForPaymentPresentation` so that it makes code searching easier (e.g. same method name in the NetworkProcess code and UIProcess code) and for the same reason as the last patch > Source/WebKit/Shared/ApplePay/ios/WebPaymentCoordinatorProxyIOS.mm:61 > + weakThis->m_authorizationPresenter->presentInScene(sceneID, WTFMove(completionHandler)); see my comment on your last patch about this > Source/WebKit/UIProcess/Network/NetworkProcessProxy.h:261 > + void getWindowSceneIdentifierForPaymentPresentation(WebPageProxyIdentifier, CompletionHandler<void(String)>&&); Can we `const String&`?
Created attachment 439684 [details] Patch
Comment on attachment 439684 [details] Patch r=me awesome work! =D
Committed r283320 (242345@main): <https://commits.webkit.org/242345@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 439684 [details].
<rdar://problem/83727523>