[Payment Request] Implement MerchantValidationEvent.methodName
Created attachment 351014 [details] Patch
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.
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.
(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!
Created attachment 351182 [details] Patch
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.
Created attachment 351188 [details] Patch
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.
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&.
(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.
Created attachment 351260 [details] Patch
(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&.
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.
Created attachment 351279 [details] Patch
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.
Created attachment 353256 [details] Patch
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.
Comment on attachment 353256 [details] Patch Clearing flags on attachment: 353256 Committed r237521: <https://trac.webkit.org/changeset/237521>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45620511>
*** Bug 189548 has been marked as a duplicate of this bug. ***