[webkit-reviews] review granted: [Bug 189100] [Payment Request] Implement the PaymentMethodChangeEvent and PaymentMethodChangeEventInit interfaces : [Attachment 348520] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 189100: [Payment Request] Implement the PaymentMethodChangeEvent and
PaymentMethodChangeEventInit interfaces
https://bugs.webkit.org/show_bug.cgi?id=189100

Attachment 348520: Patch

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




--- Comment #11 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 348520
  --> https://bugs.webkit.org/attachment.cgi?id=348520
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.