Bug 230874 – Adopt presentationSceneIdentifierForPaymentAuthorizationController delegate call from PassKit

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

Created attachment 439424 [details]
Proposed patch

PassKit offers presentationSceneIdentifierForPaymentAuthorizationController delegate call, but WebKit made no effort to respond to that.

[Devin Rousso]

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

[Devin Rousso]

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

[Devin Rousso]

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

[Devin Rousso]

Comment on attachment 439684 [details]
Patch

r=me awesome work! =D

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/83727523>