[Webkit-unassigned] [Bug 230874] Adopt presentationSceneIdentifierForPaymentAuthorizationController delegate call from PassKit

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 27 20:20:00 PDT 2021


https://bugs.webkit.org/show_bug.cgi?id=230874

--- Comment #1 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 439424
  --> https://bugs.webkit.org/attachment.cgi?id=439424
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`?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210928/4b3636b5/attachment-0001.htm>


More information about the webkit-unassigned mailing list