Bug 195219 – [Apple Pay] Untangle WebPageProxy from WebPaymentCoordinatorProxy

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Apple Pay] Untangle WebPageProxy and WebPaymentCoordinatorProxy

[Radar WebKit Bug Importer]

<rdar://problem/48518082>

[Andy Estes]

Created attachment 363354 [details]
Patch

[Darin Adler]

Comment on attachment 363354 [details]
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.

[Andy Estes]

Comment on attachment 363354 [details]
Patch

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

>> Source/WebKit/UIProcess/ApplePay/WebPaymentCoordinatorProxy.h:38
>> +#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.

I knew the #import was safe, but I didn't see a reason to prefer it over #include since the header in question uses "#pragma once." Personally I like seeing a file with either all #includes or all #imports, just as an aesthetic preference.

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

I agree with your name suggestion.

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

Ditto.

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

Ditto.

>> Source/WebKit/UIProcess/WebPageProxy.h:1989
>> +#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.

Makes sense. Will un-nest.

Thanks for reviewing!

[Andy Estes]

Created attachment 363464 [details]
Patch

[Andy Estes]

Created attachment 363465 [details]
Patch

[Andy Estes]

Created attachment 363466 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 363466 [details]
Patch

Clearing flags on attachment: 363466

Committed r242332: <https://trac.webkit.org/changeset/242332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.