[webkit-reviews] review granted: [Bug 195219] [Apple Pay] Untangle WebPageProxy from WebPaymentCoordinatorProxy : [Attachment 363354] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 195219: [Apple Pay] Untangle WebPageProxy from WebPaymentCoordinatorProxy
https://bugs.webkit.org/show_bug.cgi?id=195219

Attachment 363354: Patch

https://bugs.webkit.org/attachment.cgi?id=363354&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 363354
  --> https://bugs.webkit.org/attachment.cgi?id=363354
Patch

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

> Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:38
> -#import <WebKitAdditions/WebPaymentCoordinatorProxyAdditions.h>
> +#include <WebKitAdditions/WebPaymentCoordinatorProxyAdditions.h>

Why change this?

Here’s my argument for not changing it: #import is not specific to Objective-C,
it’s specific to the C compilers that support Objective-C and the only compiler
we will be compiling ENABLE(APPLE_PAY) code with does supports it. So it seems
safe to use #import inside that #if.

> Source/WebKit/UIProcess/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:509
> +    auto& sourceApplicationBundleIdentifier =
m_client.paymentCoordinatorSourceApplicationBundleIdentifier(*this);

Often for local variables we can use fewer words and a shorter name, because
context makes them unambiguous enough. Could this just be "bundleIdentifier" or
would that make things confusing? The context would make it clear which bundle
identifier. I wouldn’t insist: Whatever you think is clearest.

> Source/WebKit/UIProcess/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:513
> +    auto& sourceApplicationSecondaryIdentifier =
m_client.paymentCoordinatorSourceApplicationSecondaryIdentifier(*this);

Maybe "secondaryIdentifier"?

> Source/WebKit/UIProcess/ApplePay/cocoa/WebPaymentCoordinatorProxyCocoa.mm:518
> +    auto& ctDataConnectionServiceType =
m_client.paymentCoordinatorCTDataConnectionServiceType(*this);

Maybe "serviceType"?

> Source/WebKit/UIProcess/WebPageProxy.h:1989
> +#if PLATFORM(IOS_FAMILY)
> +    UIViewController *paymentCoordinatorPresentingViewController(const
WebPaymentCoordinatorProxy&) final;
> +    const String& paymentCoordinatorCTDataConnectionServiceType(const
WebPaymentCoordinatorProxy&) final;
> +#endif
> +#if PLATFORM(MAC)
> +    NSWindow *paymentCoordinatorPresentingWindow(const
WebPaymentCoordinatorProxy&) final;
> +#endif
> +#endif

I have been encouraging people to write #if statements with boolean
combinations rather than nesting #if like this. The fact that we don’t have
indentation to help us keep track of the nesting, and that the if statements
read pretty well with && make me prefer this style, and I have been using it
all over WebKit.