1375345 - [Payment Request API] Basic card payment method implementation

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

This bug is used to tracing the implementation of w3c spec 

https://www.w3.org/TR/payment-method-basic-card/

Comment 1

[Ben Tian (inactive)]

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."

Comment 2

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e89200f8f2b7904e4e621b93d29561838bb2c565

Comment 3

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bce131ecafc3569d51f4a188093c8855b2977320

Comment 4

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a74cda5423fe7369cc37e310eac44ce513364969

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1375345 - Basic card payment implementation. r?baku

This patch implements the basic card payment support for PaymentRequest API.

Comment 6

[Eden Chuang[:edenchuang]]

Attachment: Bug 1375345 - Mochitest for basic card payment implementation. r?baku

Comment 7

[Andrea Marchesini [:baku]]

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);

Comment 8

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ed21a179dff0a52753d5407afeed517d8a7cd20

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1375345 - Basic card payment implementation. r?baku

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.

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1375345 - Mochitest for basic card payment implementation. r=baku

Update the patch to resolve the conflict.

Comment 11

[Andrea Marchesini [:baku]]

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.

Comment 12

[Eden Chuang[:edenchuang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b098e4022a585cc8099f1ab45b51b4eed6c393cd

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug 1375345 - Basic card payment implementation. r=baku

Comment 14

[Eden Chuang[:edenchuang]]

Attachment: Bug 1375345 - mochitest for basic card payment implementation. r=baku

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a26a2f147e4
https://hg.mozilla.org/mozilla-central/rev/cce9237283c9

Comment 17

[Ionuț Goldan [:igoldan]]

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

Comment 18

[Ben Tian (inactive)]

Redirect ni? to owner Eden.

Comment 19

[Eden Chuang[:edenchuang]]

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.

Comment 20

[Chris Mills [:cmills]]

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!

Comment 21

[Marcos Caceres [:marcosc]]

Happy to do a technical review, but need a couple of days to do it. Is that ok?

Comment 22

[Chris Mills [:cmills]]

(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

[Marcos Caceres [:marcosc]]

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

[Chris Mills [:cmills]]

(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.