Bug 190058 – [Payment Request] Implement MerchantValidationEvent.methodName

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

[Payment Request] Implement MerchantValidationEvent.methodName

[Andy Estes]

Created attachment 351014 [details]
Patch

[EWS Watchlist]

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


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


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

[Darin Adler]

Comment on attachment 351014 [details]
Patch

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

> Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:486
> +        m_paymentRequest->dispatchEvent(MerchantValidationEvent::create(eventNames().merchantvalidationEvent, WTF::get<URL>(m_identifier).string(), WTFMove(validationURL)).get());

I must admit I don’t understand why it’s guaranteed that m_identifier is a URL, and not some other type. But I do see this idiom elsewhere in the class so it must be OK.

> Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.cpp:49
> +    if (!methodName.isEmpty() && !convertAndValidatePaymentMethodIdentifier(methodName))
> +        return Exception { RangeError, makeString('"', methodName, "\" is an invalid payment method identifier.") };

If convertAndValidatePaymentMethodIdentifier is used elsewhere, I wonder if this exception is also needed elsewhere. Could share the code.

[Andy Estes]

(In reply to Darin Adler from comment #3)
> Comment on attachment 351014 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351014&action=review
> 
> > Source/WebCore/Modules/applepay/paymentrequest/ApplePayPaymentHandler.cpp:486
> > +        m_paymentRequest->dispatchEvent(MerchantValidationEvent::create(eventNames().merchantvalidationEvent, WTF::get<URL>(m_identifier).string(), WTFMove(validationURL)).get());
> 
> I must admit I don’t understand why it’s guaranteed that m_identifier is a
> URL, and not some other type. But I do see this idiom elsewhere in the class
> so it must be OK.

Apple Pay's payment method identifier is always a URL (enforced by ApplePayPaymentHandler::handlesIdentifier).

Given that, m_identifier could just be a URL.

Alternatively, PaymentRequest::MethodIdentifier could just be a String. We only need to verify that URL-based identifiers have the correct properties (https, no username or password), but after that we can just store a string representation of the URL.

I'll think about how to make this better.

> 
> > Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.cpp:49
> > +    if (!methodName.isEmpty() && !convertAndValidatePaymentMethodIdentifier(methodName))
> > +        return Exception { RangeError, makeString('"', methodName, "\" is an invalid payment method identifier.") };
> 
> If convertAndValidatePaymentMethodIdentifier is used elsewhere, I wonder if
> this exception is also needed elsewhere. Could share the code.

Good call, I'll change this to return an ExceptionOr<PaymentRequest::MethodIdentifier>.

Thanks for reviewing!

[Andy Estes]

Created attachment 351182 [details]
Patch

[EWS Watchlist]

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


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


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

[Andy Estes]

Created attachment 351188 [details]
Patch

[EWS Watchlist]

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


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


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

[Darin Adler]

Comment on attachment 351188 [details]
Patch

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

> Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.cpp:51
> +        auto validatedMethodName = convertAndValidatePaymentMethodIdentifier(methodName);
> +        if (validatedMethodName.hasException())
> +            return validatedMethodName.releaseException();

I think there is a missing line of code here:

    methodName = validatedMethodName.releaseReturnValue();

> Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.h:45
> +    static Ref<MerchantValidationEvent> create(const AtomicString& type, String methodName, URL&& validationURL);

Surprised that this uses String, and neither String&& or const String&.

[Andy Estes]

(In reply to Darin Adler from comment #9)
> Comment on attachment 351188 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351188&action=review
> 
> > Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.cpp:51
> > +        auto validatedMethodName = convertAndValidatePaymentMethodIdentifier(methodName);
> > +        if (validatedMethodName.hasException())
> > +            return validatedMethodName.releaseException();
> 
> I think there is a missing line of code here:
> 
>     methodName = validatedMethodName.releaseReturnValue();

It's ok to use methodName as-is so long as the validation didn't raise an exception. I'm only interested in the exception, not the converted identifier.

> 
> > Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.h:45
> > +    static Ref<MerchantValidationEvent> create(const AtomicString& type, String methodName, URL&& validationURL);
> 
> Surprised that this uses String, and neither String&& or const String&.

Yeah, I guess one copy is slightly better than one copy and one move.

[Andy Estes]

Created attachment 351260 [details]
Patch

[Andy Estes]

(In reply to Andy Estes from comment #10)
> (In reply to Darin Adler from comment #9)
> > Comment on attachment 351188 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=351188&action=review
> > 
> > > Source/WebCore/Modules/paymentrequest/MerchantValidationEvent.h:45
> > > +    static Ref<MerchantValidationEvent> create(const AtomicString& type, String methodName, URL&& validationURL);
> > 
> > Surprised that this uses String, and neither String&& or const String&.
> 
> Yeah, I guess one copy is slightly better than one copy and one move.

Forgot to say: Changed to use const String&.

[EWS Watchlist]

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


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


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

[Andy Estes]

Created attachment 351279 [details]
Patch

[EWS Watchlist]

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


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


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

[Andy Estes]

Created attachment 353256 [details]
Patch

[EWS Watchlist]

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


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


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

[WebKit Commit Bot]

Comment on attachment 353256 [details]
Patch

Clearing flags on attachment: 353256

Committed r237521: <https://trac.webkit.org/changeset/237521>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45620511>

[Andy Estes]

*** Bug 189548 has been marked as a duplicate of this bug. ***