Bug 195219 - [Apple Pay] Untangle WebPageProxy from WebPaymentCoordinatorProxy
Summary: [Apple Pay] Untangle WebPageProxy from WebPaymentCoordinatorProxy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andy Estes
URL:
Keywords: InRadar
Depends on:
Blocks: 195078
  Show dependency treegraph
 
Reported: 2019-03-01 12:03 PST by Andy Estes
Modified: 2019-03-03 14:16 PST (History)
10 users (show)

See Also:


Attachments
Patch (33.71 KB, patch)
2019-03-01 12:21 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (33.60 KB, patch)
2019-03-03 12:14 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (33.60 KB, patch)
2019-03-03 12:17 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (33.55 KB, patch)
2019-03-03 12:25 PST, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2019-03-01 12:03:59 PST
[Apple Pay] Untangle WebPageProxy and WebPaymentCoordinatorProxy
Comment 1 Radar WebKit Bug Importer 2019-03-01 12:06:01 PST
<rdar://problem/48518082>
Comment 2 Andy Estes 2019-03-01 12:21:14 PST
Created attachment 363354 [details]
Patch
Comment 3 Darin Adler 2019-03-03 00:15:26 PST
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.
Comment 4 Andy Estes 2019-03-03 11:56:24 PST
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!
Comment 5 Andy Estes 2019-03-03 12:14:55 PST Comment hidden (obsolete)
Comment 6 Andy Estes 2019-03-03 12:17:01 PST Comment hidden (obsolete)
Comment 7 Andy Estes 2019-03-03 12:25:15 PST
Created attachment 363466 [details]
Patch
Comment 8 WebKit Commit Bot 2019-03-03 14:16:40 PST
Comment on attachment 363466 [details]
Patch

Clearing flags on attachment: 363466

Committed r242332: <https://trac.webkit.org/changeset/242332>
Comment 9 WebKit Commit Bot 2019-03-03 14:16:42 PST
All reviewed patches have been landed.  Closing bug.