Closed Bug 1375345 Opened 2 years ago Closed 2 years ago

[Payment Request API] Basic card payment method implementation

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set

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)

This bug is used to tracing the implementation of w3c spec 

https://www.w3.org/TR/payment-method-basic-card/
Assignee: nobody → echuang
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."
This patch implements the basic card payment support for PaymentRequest API.
Attachment #8886863 - Flags: review?(amarchesini)
Attachment #8886864 - Flags: review?(amarchesini) → review+
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-
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)
Update the patch to resolve the conflict.
Attachment #8886864 - Attachment is obsolete: true
Attachment #8888342 - Flags: review+
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+
Attachment #8888339 - Attachment is obsolete: true
Attachment #8890343 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/2a26a2f147e4
https://hg.mozilla.org/mozilla-central/rev/cce9237283c9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Alias: paymentrequest-wpt
Alias: paymentrequest-wpt
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)
Redirect ni? to owner Eden.
Flags: needinfo?(btian) → needinfo?(echuang)
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)
Whiteboard: [WP-MVP][M4]
Whiteboard: [WP-MVP][M4]
Happy to do a technical review, but need a couple of days to do it. Is that ok?
Flags: needinfo?(mcaceres)
(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.
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.
(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.