Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2410143003:
Add currencySystem field to PaymentCurrencyAmount (Closed)
|
Created:
4 years, 2 months ago by pals Modified:
4 years, 2 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, blink-reviews, haraken Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd currencySystem field to PaymentCurrencyAmount
currencySystem is a URL that indicates the currency system that the
currency identifier belongs to. By default, the value is
urn:iso:std:iso:4217 indicating that currency is defined by [[ISO4217]]
(for example, USD for US Dollars).
BUG=652147
Committed: https://crrev.com/1ec709488535f68c662795128643be5eb32d4b1c
Cr-Commit-Position: refs/heads/master@{#425849}
Patch Set 1 : Add currencySystem field to PaymentCurrencyAmount #
Total comments: 16
Patch Set 2 : Add currencySystem field to PaymentCurrencyAmount #
Total comments: 14
Patch Set 3 : Rebased #Patch Set 4 : Fix tests in Win builds #Messages
Total messages: 36 (26 generated)
Patchset #1 (id:1) has been deleted
sanjoy.pal@samsung.com changed reviewers: + rouslan@chromium.org
Please check if the approach is correct. I can add more test case later.
The approach is right. Thank you for the contribution! :-D https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:28: itemAmount.setCurrencySystem("http://www.example.com"); Please add PaymentTestDataCurrencySystem and call "setCurrencySystem()" only if that's being used. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:20: if (!currencySystemUrl.isValid()) { Inline "currencySystemUrl": if (!KURL(KURL(), system).isValid()) { ... } https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:21: if (optionalErrorMessage) { Let's use {} in the same way as the rest of the file. If statements with single line bodies don't need {}. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:28: if (system == "urn:iso:std:iso:4217") { This is a URN, which is different from URL. Therefore, check for this URN before you call KURL.isValid(). https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:32: if (optionalErrorMessage) { No need for {} https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.h:20: // however any string of at most 2048 characters is considered valid. Returns false if currency |code| is too long (greater than 2048). Please update the comments to talk about different validation rules for ISO4217 and other systems. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp:14: struct TestCase { Please move the "TestCase" down to be immediately above the first place where it's used. (Line 100-ish.) https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp:77: CurrencyCodeTestCase("US1", "http://www.example.com", true), For each of the valid example.com currency systems test cases, please also add one test case for "urn:iso:std:iso:4217" that is invalid. For example, add "US1"/"urn:iso:std:iso:4217"/false test case on this line.
Fixed the comments. PTAL. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:28: itemAmount.setCurrencySystem("http://www.example.com"); On 2016/10/12 23:35:49, rouslan (slow 10.10-10.17) wrote: > Please add PaymentTestDataCurrencySystem and call "setCurrencySystem()" only if > that's being used. Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:20: if (!currencySystemUrl.isValid()) { On 2016/10/12 23:35:50, rouslan (slow 10.10-10.17) wrote: > Inline "currencySystemUrl": > > if (!KURL(KURL(), system).isValid()) { > ... > } Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:21: if (optionalErrorMessage) { On 2016/10/12 23:35:49, rouslan (slow 10.10-10.17) wrote: > Let's use {} in the same way as the rest of the file. If statements with single > line bodies don't need {}. Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:28: if (system == "urn:iso:std:iso:4217") { On 2016/10/12 23:35:50, rouslan (slow 10.10-10.17) wrote: > This is a URN, which is different from URL. Therefore, check for this URN before > you call KURL.isValid(). Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:32: if (optionalErrorMessage) { On 2016/10/12 23:35:50, rouslan (slow 10.10-10.17) wrote: > No need for {} Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidators.h (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.h:20: // however any string of at most 2048 characters is considered valid. Returns false if currency |code| is too long (greater than 2048). On 2016/10/12 23:35:50, rouslan (slow 10.10-10.17) wrote: > Please update the comments to talk about different validation rules for ISO4217 > and other systems. Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp (right): https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp:14: struct TestCase { On 2016/10/12 23:35:50, rouslan (slow 10.10-10.17) wrote: > Please move the "TestCase" down to be immediately above the first place where > it's used. (Line 100-ish.) Done. https://codereview.chromium.org/2410143003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidatorsTest.cpp:77: CurrencyCodeTestCase("US1", "http://www.example.com", true), On 2016/10/12 23:35:50, rouslan (slow 10.10-10.17) wrote: > For each of the valid http://example.com currency systems test cases, please also add > one test case for "urn:iso:std:iso:4217" that is invalid. For example, add > "US1"/"urn:iso:std:iso:4217"/false test case on this line. Done.
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sanjoy.pal@samsung.com
The bots are failing with patch failures. Do you need to rebase? https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp (left): https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:340: ""), Please remove lines 317-340. These tests are no longer correct. If you want, you can add some test cases like this: INSTANTIATE_TEST_CASE_P( ValidCurrencySystem, PaymentRequestDetailsTest, testing::Values(DetailsTestCase(PaymentTestDetailTotal, PaymentTestDataCurrencySystem, PaymentTestOverwriteValue, "https://bitcoin.org")))); INSTANTIATE_TEST_CASE_P( InvalidCurrencySystem, PaymentRequestDetailsTest, testing::Values(DetailsTestCase(PaymentTestDetailTotal, PaymentTestDataCurrencySystem, PaymentTestOverwriteValue, "\\\\")))); https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:369: ""), Remove lines 346-369. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:398: ""), Remove lines 375-398. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:427: ""), Remove lines 404-427. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:456: ""))); Remove lines 433-456. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:33: itemAmount.setCurrencySystem("http://www.example.com"); // Currency system is "urn:iso:std:iso:4217" by default. if (data == PaymentTestDataCurrencySystem) { if (modificationType == PaymentTestOverwriteValue) itemAmount.setCurrency(valueToUse); else itemAmount.setCurrency(String()); // null string. } https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp (right): https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:33: *optionalErrorMessage = "The currency system is not a valid url"; Upper-case "URL" please.
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fixed the comments. PTAL. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp (left): https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:340: ""), On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > Please remove lines 317-340. These tests are no longer correct. If you want, you > can add some test cases like this: > > INSTANTIATE_TEST_CASE_P( > ValidCurrencySystem, > PaymentRequestDetailsTest, > testing::Values(DetailsTestCase(PaymentTestDetailTotal, > PaymentTestDataCurrencySystem, > PaymentTestOverwriteValue, > "https://bitcoin.org")))); > > INSTANTIATE_TEST_CASE_P( > InvalidCurrencySystem, > PaymentRequestDetailsTest, > testing::Values(DetailsTestCase(PaymentTestDetailTotal, > PaymentTestDataCurrencySystem, > PaymentTestOverwriteValue, > "\\\\")))); Done. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:369: ""), On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > Remove lines 346-369. Done. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:398: ""), On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > Remove lines 375-398. Done. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:427: ""), On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > Remove lines 404-427. Done. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequestDetailsTest.cpp:456: ""))); On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > Remove lines 433-456. Done. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp (right): https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentTestHelper.cpp:33: itemAmount.setCurrencySystem("http://www.example.com"); On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > // Currency system is "urn:iso:std:iso:4217" by default. > if (data == PaymentTestDataCurrencySystem) { > if (modificationType == PaymentTestOverwriteValue) > itemAmount.setCurrency(valueToUse); > else > itemAmount.setCurrency(String()); // null string. > } Done. https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp (right): https://codereview.chromium.org/2410143003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentsValidators.cpp:33: *optionalErrorMessage = "The currency system is not a valid url"; On 2016/10/13 17:37:54, rouslan (slow 10.10-10.17) wrote: > Upper-case "URL" please. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sanjoy.pal@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Updated the test for win builds. PTAL.
lgtm
The CQ bit was checked by sanjoy.pal@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Add currencySystem field to PaymentCurrencyAmount currencySystem is a URL that indicates the currency system that the currency identifier belongs to. By default, the value is urn:iso:std:iso:4217 indicating that currency is defined by [[ISO4217]] (for example, USD for US Dollars). BUG=652147 ========== to ========== Add currencySystem field to PaymentCurrencyAmount currencySystem is a URL that indicates the currency system that the currency identifier belongs to. By default, the value is urn:iso:std:iso:4217 indicating that currency is defined by [[ISO4217]] (for example, USD for US Dollars). BUG=652147 Committed: https://crrev.com/1ec709488535f68c662795128643be5eb32d4b1c Cr-Commit-Position: refs/heads/master@{#425849} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1ec709488535f68c662795128643be5eb32d4b1c Cr-Commit-Position: refs/heads/master@{#425849} |
