1388661 - Pass payment-request-ctor-currency-code-checks web-platform-tests.

Status: RESOLVED FIXED

(Core :: DOM: Web Payments, enhancement)

Description

[Eden Chuang[:edenchuang]]

Need to pass the web-platform-tests for currency checking.

Comment 1

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Support currency validation in PaymentRequest API. r?baku

Comment 2

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Support currency validation in PaymentRequest API. r?baku

Support currency validation in Constructor() and PaymentReqeustUpdateEvent::updateWith().

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Mochitest for currency validation in PaymentRequest API. r?baku

Comment 4

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Support currency validation in PaymentRequet API. r?baku

This patch implements currency validation algorithm according to the spec https://w3c.github.io/payment-request/#validity-checkers.
    1. amount.currencySystem must be "urn:iso:std:iso:4217".                     
    2. amount.currency is valid with following criteria                          
         1. The currency length must be 3.
         2. The currency contains any character that must be in the range "A" to   
            "Z"(U+0041 to U+005A) or the range "a" to "z"(U+0061 to U+007A).       

According to the spec, converting the currency to upper case and save it in nsIPaymentRequest.

Comment 5

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Mochitest for PaymentRequest API currency amount validation. r?baku

1. Create a new test test_currency_amount_validation.html to test validation
       with following scenarios
       * test with well-formed currency codes.
       * test with invalid currency codes.
       * test with valid lower case currency codes and check is it converted to
         upper case.
       * test with invalid currency codes while calling
         PaymentRequestUpdateEvent::updateWith().
       * test with invalid amount value with calling
         PaymentRequestUpdateEvent::updateWith().
    2. Move tests of test_validate_decimal_value.html to
       test_currency_amount_validation.html

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8901736 [details] [diff] [review]
Bug 1388661 - Support currency validation in PaymentRequet API. r?baku

Review of attachment 8901736 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/PaymentRequest.cpp
@@ +167,5 @@
> +    *  2. The currency contains any character that must be in the range "A" to "Z"
> +    *     (U+0041 to U+005A) or the range "a" to "z" (U+0061 to U+007A)
> +    */
> +   nsAutoString errMsg;
> +   errMsg.AssignLiteral("The amount.currency of \"");

No needs to create this error message if the validation succeeds.
What about:

if (aCurrency.Length() != 3) {
  aErrorMsg.AssignLiteral("The length amount.currency of \"");
  aErMsg.Append(aItem);
  aErrMsg.AppendLiteral("\"(");
  aErrMsg.Append(aCurrency);
  aErrMsg.AppendLiteral(") must be 3.");
  return NS_ERROR_RANGE_ERR;
}

for (...
  if ( ...
    aErrorMsg.AssignLiteral("The amount.currency of \"");
    aErMsg.Append(aItem);
    aErrMsg.AppendLiteral("\"(");
    aErrMsg.Append(aCurrency);
   aErrMsg.AppendLiteral(") must...");

@@ +362,5 @@
>    } while (node);
>  
>    // Check payment methods and details
>    nsAutoString message;
> +  nsresult rv = IsValidMethodData(nsContentUtils::GetCurrentJSContext(),

Can you take the JSContext from aGlobal? Just do: aGlobal.Context()

Please check if we do the same elsewhere.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8901738 [details] [diff] [review]
Bug 1388661 - Mochitest for PaymentRequest API currency amount validation. r?baku

Review of attachment 8901738 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/payments/test/test_currency_amount_validation.html
@@ +55,5 @@
> +    "ınr",
> +    "invalid",
> +    "in",
> +    "123",
> +    "",

duplicate.

@@ +101,5 @@
> +    {
> +      toString() {
> +        return " 1.0";
> +      },
> +    },

{ toString() { throw "42"; }

check this as well.

Comment 8

[Eden Chuang[:edenchuang]]

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

Comment 9

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Support currency validation in PaymentRequet API. r=baku

Fix the patch according to comment 6.

1. remove the pre-generated error message.
2. replace all nsContentUtils::GetCurrentJSContext() with aGlobal.Context().

Comment 10

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Mochitest for currency validation in PaymentRequest API. r=baku

Fix the patch according to the comment 7.

1. remove duplicate invalid amount "".
2. add an invalid amount toString() { throw "42"; }.

Comment 11

[Eden Chuang[:edenchuang]]

remove checkin-needed for update the right patches.

Comment 12

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Support currency validation in PaymentRequet API. r=baku

Fix the patch according to comment 6.

1. remove the pre-generated error message in IsValidCurrency.
2. replace all nsContentUtils::GetCurrentJSContext() with aGlobal.Context().

Comment 13

[Eden Chuang[:edenchuang]]

Attachment: Bug 1388661 - Mochitest for currency validation in PaymentRequest API. r=baku

Fix the patch according to comment 7.

1. remove duplicated invalid amount value "".
2. add a new invalid amount value toString() { throw "42"; }.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18ee8840162
Support currency validation in PaymentRequet API. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/89e526b3ec99
Mochitest for PaymentRequest API currency amount validation. r=baku

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f18ee8840162
https://hg.mozilla.org/mozilla-central/rev/89e526b3ec99