Bug 189100

Comment on attachment 348439 [details]
Patch

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

> LayoutTests/imported/w3c/ChangeLog:11
> +        One test in onpaymentmenthodchange-attribute.https.html continues to fail due to a bug in
> +        the test.

Oops! This is a bug in the patch, not the test. Fixing.

Comment on attachment 348520 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:9
> +        by <https://w3c.github.io/payment-request/#paymentmethodchangeevent-interface>.

For historical preservation, please include the date of the draft you referenced.

> Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:27
> +#include "PaymentMethodChangeEvent.h"

I suggest we move this under the ENABLE(PAYMENT_REQUEST)-guard.

> Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:40
> +PaymentMethodChangeEvent::PaymentMethodChangeEvent(const AtomicString& type, const PaymentMethodChangeEventInit& eventInit)

Can we make the first parameter an rvalue reference and move it into PaymentRequestUpdateEvent?

> Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.h:40
> +struct PaymentMethodChangeEventInit;

This forward declaration is unnecessary as PaymentRequestUpdateEvent.h already contains it.

> Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.h:44
> +    template <typename... Args> static Ref<PaymentMethodChangeEvent> create(Args&&... args)

Nit: We do not put a space between "template" and the template parameters. The code base has a mix of both kinds. The clear majority is without the space.

> Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEventInit.h:39
> +struct PaymentMethodChangeEventInit final : public PaymentRequestUpdateEventInit {

The keyword "public" is not necessary.

> Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEventInit.h:34
> +struct PaymentRequestUpdateEventInit : public EventInit {

Ditto.

(In reply to Daniel Bates from comment #11)
> Comment on attachment 348520 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348520&action=review
> 
> r=me
> 
> > Source/WebCore/ChangeLog:9
> > +        by <https://w3c.github.io/payment-request/#paymentmethodchangeevent-interface>.
> 
> For historical preservation, please include the date of the draft you
> referenced.

Ok.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:27
> > +#include "PaymentMethodChangeEvent.h"
> 
> I suggest we move this under the ENABLE(PAYMENT_REQUEST)-guard.

Ok.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:40
> > +PaymentMethodChangeEvent::PaymentMethodChangeEvent(const AtomicString& type, const PaymentMethodChangeEventInit& eventInit)
> 
> Can we make the first parameter an rvalue reference and move it into
> PaymentRequestUpdateEvent?

I don't think so, because then I'd initialize m_methodName and m_methodDetails using a moved-from value.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.h:40
> > +struct PaymentMethodChangeEventInit;
> 
> This forward declaration is unnecessary as PaymentRequestUpdateEvent.h
> already contains it.

PaymentRequestUpdateEvent.h contains a forward declaration of PaymentRequestUpdateEventInit, not PaymentMethodChangeEventInit.

> 
> > Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.h:44
> > +    template <typename... Args> static Ref<PaymentMethodChangeEvent> create(Args&&... args)
> 
> Nit: We do not put a space between "template" and the template parameters.
> The code base has a mix of both kinds. The clear majority is without the
> space.
> 
> > Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEventInit.h:39
> > +struct PaymentMethodChangeEventInit final : public PaymentRequestUpdateEventInit {
> 
> The keyword "public" is not necessary.
> 
> > Source/WebCore/Modules/paymentrequest/PaymentRequestUpdateEventInit.h:34
> > +struct PaymentRequestUpdateEventInit : public EventInit {
> 
> Ditto.

Fixed.

Created attachment 348527 [details]
Patch

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


ERROR: Source/WebCore/Modules/paymentrequest/PaymentMethodChangeEvent.cpp:42:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 21 files


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

Comment on attachment 348527 [details]
Patch

Clearing flags on attachment: 348527

Committed r235518: <https://trac.webkit.org/changeset/235518>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/43941938>