Bug 165860 – [ApplePay] Remove remaining custom bindings from the ApplePay code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

[ApplePay] Remove remaining custom bindings from the ApplePay code

[Sam Weinig]

Created attachment 297114 [details]
Patch

[WebKit Commit Bot]

Attachment 297114 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/applepay/ApplePaySession.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 297124 [details]
Patch

[WebKit Commit Bot]

Attachment 297124 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/applepay/ApplePaySession.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 297124 [details]
Patch

Attachment 297124 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2723621

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/interfaces.worker.html

[Build Bot]

Created attachment 297137 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 297124 [details]
Patch

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

Didn’t finish reviewing, but here are my first few comments.

> Source/WebCore/Modules/applepay/ApplePayLineItem.h:36
> +    using Type = WebCore::PaymentRequest::LineItem::Type;

I don’t think the WebCore prefix is needed here.

> Source/WebCore/Modules/applepay/ApplePayLineItem.idl:31
> +[
> +    Conditional=APPLE_PAY,
> +] enum ApplePayLineItemType {
> +    "pending",
> +    "final"
> +};

Bad to rely on this, I suppose, but a conditional applied to the dictionary in this file applies to the entire file and does not need to be repeated on the enum.

> Source/WebCore/Modules/applepay/ApplePayPaymentRequest.h:37
> +#include <heap/Strong.h>
> +#include <wtf/Optional.h>
> +#include <wtf/Vector.h>
> +#include <wtf/text/WTFString.h>

Are any of these redundant? I think some of the other headers above already include these.

> Source/WebCore/Modules/applepay/ApplePayPaymentRequest.h:45
> +    using ShippingType = WebCore::PaymentRequest::ShippingType;

I don’t think you need the WebCore prefix here.

> Source/WebCore/Modules/applepay/ApplePayPaymentToken.h:31
> +#include <wtf/text/WTFString.h>

No need for this include.

[Sam Weinig]

Created attachment 297196 [details]
Patch

[WebKit Commit Bot]

Attachment 297196 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/applepay/ApplePaySession.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 297236 [details]
Patch

[WebKit Commit Bot]

Attachment 297236 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/applepay/ApplePaySession.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 297247 [details]
Patch

[WebKit Commit Bot]

Attachment 297247 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/applepay/ApplePaySession.h:137:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 297247 [details]
Patch

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

> Source/WebCore/Modules/applepay/ApplePayPaymentPass.h:35
> +namespace WebCore {
> +
> +
> +struct ApplePayPaymentPass {

Extra blank line here.

> Source/WebCore/Modules/applepay/ApplePaySession.h:46
>  class URL;
> +struct ApplePayLineItem;

I normally use a blank line and put the structs into a separate paragraph from the classes.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4548
> -    push(@headerContent, GenerateDictionariesHeaderContent(undef, $otherDictionaries)) if $otherDictionaries;
> +    push(@headerContent, GenerateDictionariesHeaderContent($dictionary, $otherDictionaries)) if $otherDictionaries;

I would expect this change to break the build, because MediaTrackConstraints does not contain a type named MediaTrackConstrants::ConstraintSet or MediaTrackConstrants::ConstrainLong. Why doesn’t the change cause a problem?

[Sam Weinig]

(In reply to comment #14)
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4548
> > -    push(@headerContent, GenerateDictionariesHeaderContent(undef, $otherDictionaries)) if $otherDictionaries;
> > +    push(@headerContent, GenerateDictionariesHeaderContent($dictionary, $otherDictionaries)) if $otherDictionaries;
> 
> I would expect this change to break the build, because MediaTrackConstraints
> does not contain a type named MediaTrackConstrants::ConstraintSet or
> MediaTrackConstrants::ConstrainLong. Why doesn’t the change cause a problem?

I explicitly set the ImplementedAs extended attribute on the MediaTrackConstrants types to avoid the issue.

[Sam Weinig]

Committed revision 209927.

[Darin Adler]

(In reply to comment #15)
> I explicitly set the ImplementedAs extended attribute on the
> MediaTrackConstrants types to avoid the issue.

Ah! I went looking but missed that.

I guess I’m not 100% thrilled with the rule we made that removes prefixes and nests types inside other types, but it seems like something we can easily refine over time.