Closed Bug 1388661 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Web Payments, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: edenchuang, Assigned: edenchuang)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

33.47 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
19.27 KB, patch
edenchuang
: review+
Details | Diff | Splinter Review
Need to pass the web-platform-tests for currency checking.
Assignee: nobody → echuang
Status: NEW → ASSIGNED
Support currency validation in Constructor() and PaymentReqeustUpdateEvent::updateWith().
Attachment #8898278 - Attachment is obsolete: true
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.
Attachment #8898674 - Attachment is obsolete: true
Attachment #8901736 - Flags: review?(amarchesini)
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
Attachment #8898675 - Attachment is obsolete: true
Attachment #8901738 - Flags: review?(amarchesini)
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.
Attachment #8901736 - Flags: review?(amarchesini) → review+
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.
Attachment #8901738 - Flags: review?(amarchesini) → review+
Fix the patch according to comment 6.

1. remove the pre-generated error message.
2. replace all nsContentUtils::GetCurrentJSContext() with aGlobal.Context().
Attachment #8901736 - Attachment is obsolete: true
Attachment #8904873 - Flags: review+
Fix the patch according to the comment 7.

1. remove duplicate invalid amount "".
2. add an invalid amount toString() { throw "42"; }.
Attachment #8901738 - Attachment is obsolete: true
Attachment #8904874 - Flags: review+
Keywords: checkin-needed
remove checkin-needed for update the right patches.
Keywords: checkin-needed
Fix the patch according to comment 6.

1. remove the pre-generated error message in IsValidCurrency.
2. replace all nsContentUtils::GetCurrentJSContext() with aGlobal.Context().
Attachment #8904873 - Attachment is obsolete: true
Attachment #8904880 - Flags: review+
Fix the patch according to comment 7.

1. remove duplicated invalid amount value "".
2. add a new invalid amount value toString() { throw "42"; }.
Attachment #8904874 - Attachment is obsolete: true
Attachment #8904881 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f18ee8840162
https://hg.mozilla.org/mozilla-central/rev/89e526b3ec99
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.