Closed
Bug 1375345
Opened 7 years ago
Closed 7 years ago
[Payment Request API] Basic card payment method implementation
Categories
(Core :: DOM: Web Payments, enhancement)
Core
DOM: Web Payments
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: edenchuang, Assigned: edenchuang)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 4 obsolete files)
53.44 KB,
patch
|
edenchuang
:
review+
|
Details | Diff | Splinter Review |
52.91 KB,
patch
|
edenchuang
:
review+
|
Details | Diff | Splinter Review |
This bug is used to tracing the implementation of w3c spec https://www.w3.org/TR/payment-method-basic-card/
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → echuang
Updated•7 years ago
|
Blocks: paymentrequest
Comment 1•7 years ago
|
||
Please remember to verify with web platform test of PaymentRequest.canMakePayment. Test 1 & 4 [1] fail now for lack of basic-card support. https://w3c-test.org/payment-request/payment-request-canmakepayment-method.https.html [1] "If request.[[state]] is "created", then return a promise that resolves to true for known method." and "If payment method identifier and serialized parts are supported, resolve promise with true."
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e89200f8f2b7904e4e621b93d29561838bb2c565
Assignee | ||
Comment 3•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce131ecafc3569d51f4a188093c8855b2977320
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a74cda5423fe7369cc37e310eac44ce513364969
Assignee | ||
Comment 5•7 years ago
|
||
This patch implements the basic card payment support for PaymentRequest API.
Attachment #8886863 -
Flags: review?(amarchesini)
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8886864 -
Flags: review?(amarchesini)
Updated•7 years ago
|
Blocks: paymentrequest-wpt
Updated•7 years ago
|
Attachment #8886864 -
Flags: review?(amarchesini) → review+
Comment 7•7 years ago
|
||
Comment on attachment 8886863 [details] [diff] [review] Bug 1375345 - Basic card payment implementation. r?baku Review of attachment 8886863 [details] [diff] [review]: ----------------------------------------------------------------- try to use macros in order to reduce duplicate code in BasicCardPayment. ::: dom/payments/BasicCardPayment.cpp @@ +45,5 @@ > + , recipientKey(NS_LITERAL_STRING("recipient")) > + , phoneKey(NS_LITERAL_STRING("phone")) > + , propertySpliter(NS_LITERAL_STRING(";")) > + , keyValueSpliter(NS_LITERAL_STRING(":")) > + , addressLineSpliter(NS_LITERAL_STRING("%")) why all of these? @@ +88,5 @@ > + JS::RootedValue data(aCx, JS::ObjectValue(*aData)); > + JS::HandleValue dataHandle(&data); > + > + BasicCardRequest request; > + if (!request.Init(aCx, dataHandle)) { you don't need the HandleValue here... just pass data. @@ +110,5 @@ > +{ > + // ExpiryMonth can only be > + // 1. empty string > + // 2. 01 ~ 12 > + if (!aExpiryMonth.IsEmpty()) { do an quick return here. If (isEmpty() => return true. @@ +115,5 @@ > + if (aExpiryMonth.Length() != 2) { > + return false; > + } > + // can only be 0X or 1X > + if (aExpiryMonth.CharAt(0) != '0' && aExpiryMonth.CharAt(0) != '1') { if you do a return if IsEmpty(), here you can do: if (aExpiryMonth.CharAt(0) == '0') { if (aExpiryMonth.CharAt(1) < '0' || aExpiryMonth.CharAt(1) > '9') { return false; } return true; } if (aExpiryMonth.CharAt(0) == '1') { if (aExpiryMonth.CharAt(1) < '0' || aExpiryMonth.CharAt(1) > '2') { return false; } return true; } return false; @@ +165,5 @@ > + nsIPaymentAddress* aBillingAddress, > + nsAString& aResult) > +{ > + // aBillingAddress can be nullptr > + nsString data; nsAutoString @@ +170,5 @@ > + nsDataHashtable<nsStringHashKey, nsString> propertiesHash; > + > + if (!aCardholderName.IsEmpty()) { > + data = aCardholderName; > + if (!propertiesHash.Put(cardholderNameKey, data, mozilla::fallible)) { use some consts here instead using members. @@ +209,5 @@ > + nsCOMPtr<nsIArray> addressLine; > + NS_ENSURE_SUCCESS(aBillingAddress->GetAddressLine(getter_AddRefs(addressLine)), > + NS_ERROR_FAILURE); > + uint32_t length; > + nsString addressLineString; nsAutoString @@ +214,5 @@ > + NS_ENSURE_SUCCESS(addressLine->GetLength(&length), NS_ERROR_FAILURE); > + for (uint32_t index = 0; index < length; ++index) { > + nsCOMPtr<nsISupportsString> address = do_QueryElementAt(addressLine, index); > + MOZ_ASSERT(address); > + nsString addressString; nsAutoString ::: dom/payments/BasicCardPayment.h @@ +47,5 @@ > + BasicCardService(); > + ~BasicCardService() = default; > + > + // key for basic card data > + const nsString cardholderNameKey; not foo, but mFoo ::: dom/payments/PaymentRequest.cpp @@ +71,5 @@ > + if (service->IsBasicCardPayment(methodData.mSupportedMethods)) { > + if (!methodData.mData.WasPassed()) { > + continue; > + } > + if (!service->IsValidBasicCardRequest(nsContentUtils::GetCurrentJSContext(), can you pass the JSContext to this method? ::: dom/payments/PaymentRequestUtils.cpp @@ +54,5 @@ > return NS_OK; > } > > +nsresult > +DeserializeToJSValue(const nsAString& aSerializedObject, JSContext* aCx, JS::MutableHandleValue aValue) { { in a new line. MOZ_ASSERT(aCx);
Attachment #8886863 -
Flags: review?(amarchesini) → review-
Updated•7 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed21a179dff0a52753d5407afeed517d8a7cd20
Assignee | ||
Comment 9•7 years ago
|
||
Modify the patch according to the review in comment 7. 1. Using macro in BasicCardService to reduce the duplicate code. 2. Quick return in IsValidExpiryMonth.
Attachment #8886863 -
Attachment is obsolete: true
Attachment #8888339 -
Flags: review?(amarchesini)
Assignee | ||
Comment 10•7 years ago
|
||
Update the patch to resolve the conflict.
Attachment #8886864 -
Attachment is obsolete: true
Attachment #8888342 -
Flags: review+
Comment 11•7 years ago
|
||
Comment on attachment 8888339 [details] [diff] [review] Bug 1375345 - Basic card payment implementation. r?baku Review of attachment 8888339 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the comments applied. ::: dom/payments/BasicCardPayment.cpp @@ +31,5 @@ > +const nsAutoString Recipient(NS_LITERAL_STRING("recipient")); > +const nsAutoString Phone(NS_LITERAL_STRING("phone")); > +const nsAutoString PropertySpliter(NS_LITERAL_STRING(";")); > +const nsAutoString KeyValueSpliter(NS_LITERAL_STRING(":")); > +const nsAutoString AddressLineSpliter(NS_LITERAL_STRING("%")); What I meant was: #define CityValue NS_LITERAL_STRING("city") #define PostalCodeValue ... @@ +48,5 @@ > + } \ > + } while(0) > + > +#define EncodeAddressLine(aAddress, aResult) \ > + do { \ You use this only once. don't do a macro for this. @@ +131,5 @@ > + mBasicCardKeys.AppendElement(CardholderName); > + mBasicCardKeys.AppendElement(CardNumber); > + mBasicCardKeys.AppendElement(ExpiryMonth); > + mBasicCardKeys.AppendElement(ExpiryYear); > + mBasicCardKeys.AppendElement(CardSecurityCode); Instead having mBasicCardKeys, you can do: bool IsBasicCardKeys(const nsAString& aValue) { return CardholderValue.Equal(aValue) || CardNumberValue.Equal(aValue) || ... } same for mAddressKeys. @@ +152,5 @@ > + mValidNetworks.AppendElement(NS_LITERAL_STRING("jcb")); > + mValidNetworks.AppendElement(NS_LITERAL_STRING("mastercard")); > + mValidNetworks.AppendElement(NS_LITERAL_STRING("mir")); > + mValidNetworks.AppendElement(NS_LITERAL_STRING("unionpay")); > + mValidNetworks.AppendElement(NS_LITERAL_STRING("visa")); same here. Just make an anonymous function. @@ +179,5 @@ > + } > + > + if (request.mSupportedNetworks.WasPassed()) { > + for (const nsString& network : request.mSupportedNetworks.Value()) { > + if (!mValidNetworks.Contains(network)) { isValidNetworks(network) ... @@ +298,5 @@ > + nsCharSeparatedTokenizer keyValueTokenizer(property, KeyValueSpliter.CharAt(0)); > + MOZ_ASSERT(keyValueTokenizer.hasMoreTokens()); > + nsDependentSubstring key = keyValueTokenizer.nextToken(); > + nsDependentSubstring value = keyValueTokenizer.nextToken(); > + if (mAddressKeys.Contains(key) && !isBillingAddressPassed) { do the comparison just once: bool isAddressKey = IsAddressKey(key); if (isAddressKey && ... if (!isAddressKey && ... ::: dom/payments/BasicCardPayment.h @@ +48,5 @@ > + ~BasicCardService() = default; > + > + nsTArray<nsString> mBasicCardKeys; > + nsTArray<nsString> mAddressKeys; > + nsTArray<nsString> mValidNetworks; you don't need these 3. ::: dom/payments/PaymentRequestService.cpp @@ +495,5 @@ > + nsCOMPtr<nsIPaymentRequest> payment; > + nsresult rv = GetPaymentRequestById(aRequestId, getter_AddRefs(payment)); > + NS_ENSURE_SUCCESS(rv, false); > + nsCOMPtr<nsIArray> methods; > + rv = payment->GetPaymentMethods(getter_AddRefs(methods)); you are not checking the return value here. @@ +505,5 @@ > + MOZ_ASSERT(method); > + nsAutoString supportedMethods; > + rv = method->GetSupportedMethods(supportedMethods); > + NS_ENSURE_SUCCESS(rv, false); > + RefPtr<BasicCardService> service = BasicCardService::GetService(); move this out of the cycle.
Attachment #8888339 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b098e4022a585cc8099f1ab45b51b4eed6c393cd
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8888339 -
Attachment is obsolete: true
Attachment #8890343 -
Flags: review+
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8888342 -
Attachment is obsolete: true
Attachment #8890344 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a26a2f147e4 Basic card payment implementation. r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/cce9237283c9 Mochitests for basic card payment implementation. r=baku
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2a26a2f147e4 https://hg.mozilla.org/mozilla-central/rev/cce9237283c9
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•7 years ago
|
Alias: paymentrequest-wpt
Updated•7 years ago
|
Alias: paymentrequest-wpt
Comment 17•7 years ago
|
||
This introduced some compiler warnings. :btian If you have the time, can you look over these and address them? == Change summary for alert #8317 (as of July 26 2017 13:59 UTC) == Regressions: 2% compiler warnings summary linux64 pgo 1,512.75 -> 1,547.17 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8317
Flags: needinfo?(btian)
Assignee | ||
Comment 19•7 years ago
|
||
I compared the compiler warnings in two logs, I found all new warnings are related about uninitialized function arguments in WebGLRenderingContextBinding.cpp. I guess the warnings might be caused by other patches. I also found that the warnings number is reduced back to 1512 after July 31.
Flags: needinfo?(echuang)
Updated•7 years ago
|
Whiteboard: [WP-MVP][M4]
Updated•7 years ago
|
Whiteboard: [WP-MVP][M4]
Comment 20•7 years ago
|
||
I have documented the two main dictionaries: https://developer.mozilla.org/en-US/docs/Web/API/BasicCardRequest https://developer.mozilla.org/en-US/docs/Web/API/BasicCardResponse (https://developer.mozilla.org/en-US/docs/Web/API/BasicCardRequest/supportedTypes mentions the BasicCardType enum as well) I have also mentioned where these objects are encountered in appropriate places on the Payment Request API: https://developer.mozilla.org/en-US/docs/Web/API/PaymentResponse https://developer.mozilla.org/en-US/docs/Web/API/PaymentResponse/details https://developer.mozilla.org/en-US/docs/Web/API/PaymentRequest/PaymentRequest and mentioned them in other appropriate places: https://developer.mozilla.org/en-US/Firefox/Experimental_features https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/features_restricted_to_secure_contexts I've not put them on the rel notes, as they are currently behind a flag. A tech review would be nice. Cheers!
Flags: needinfo?(mcaceres)
Keywords: dev-doc-needed → dev-doc-complete
Comment 21•7 years ago
|
||
Happy to do a technical review, but need a couple of days to do it. Is that ok?
Flags: needinfo?(mcaceres)
Comment 22•7 years ago
|
||
(In reply to Marcos Caceres [:marcosc] from comment #21) > Happy to do a technical review, but need a couple of days to do it. Is that > ok? Yup, that's fine.
Comment 23•7 years ago
|
||
Ok, I reviewed and fixed a few things. Could you drop the examples from `PaymentAddress`? It's quite contrived and encourages bad practice (duplicating a lot of data). Any idea why the syntax highlighting is not working on BasicCardRequest? Can we also drop? https://developer.mozilla.org/en-US/docs/Web/API/PaymentAddress/careOf I don't think that was ever formally part of the spec.
Comment 24•7 years ago
|
||
(In reply to Marcos Caceres [:marcosc] from comment #23) > Ok, I reviewed and fixed a few things. Cool, thanks Marcos! > > Could you drop the examples from `PaymentAddress`? It's quite contrived and > encourages bad practice (duplicating a lot of data). Agreed. I've removed the examples from the member pages, and added an example to the interface page that shows how the interface is seen in use. > > Any idea why the syntax highlighting is not working on BasicCardRequest? No. That is really weird. I can't see why it isn't being applied either. > > Can we also drop? > https://developer.mozilla.org/en-US/docs/Web/API/PaymentAddress/careOf > > I don't think that was ever formally part of the spec. OK. Deleted.
You need to log in
before you can comment on or make changes to this bug.
Description
•