Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1981903002:
PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl. (Closed)
DescriptionPaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl.
According to the references[1][2], 'currencyCode' property is renamed to
'currency' to allow flexible currency identifiers.
[1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currencyamount
[2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b41cdd8810d932
BUG=none
Committed: https://crrev.com/c728815891eed7f0fa5d6d7d69376ce2c2e79841
Cr-Commit-Position: refs/heads/master@{#394822}
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : #Messages
Total messages: 39 (14 generated)
Description was changed from ========== PaymentRequest: Fix a typo in CurrencyAmount.idl According to the spec[1], 'currencyCode' should be 'currency'. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... BUG=none ========== to ========== PaymentRequest: Fix a typo in CurrencyAmount.idl According to the spec[1], 'currencyCode' should be 'currency'. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... BUG=none ==========
jinho.bang@samsung.com changed reviewers: + mek@chromium.org, rouslan@chromium.org
PTAL.
Thank you for the patch! It used to be "currencyCode" initially, but I have not had the chance to update the code, while the spec has been evolving. Therefore, please change the description to "Update CurrencyAount.idl according to the new spec" or something in that flavor. Also please note that you missed a couple of files in: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a...
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981903002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981903002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== PaymentRequest: Fix a typo in CurrencyAmount.idl According to the spec[1], 'currencyCode' should be 'currency'. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... BUG=none ========== to ========== PaymentRequest: Update CurrencyAmount.idl according to the latest spec. According to the spec change[1][2], 'currencyCode' property should be 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none ==========
On 2016/05/16 16:06:37, Rouslan wrote: > Thank you for the patch! > > It used to be "currencyCode" initially, but I have not had the chance to update > the code, while the spec has been evolving. Therefore, please change the > description to "Update CurrencyAount.idl according to the new spec" or something > in that flavor. Also please note that you missed a couple of files in: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a... Thank you for review! I've just uploaded a new patch set and changed the description. Could you review it again? Thank you.
https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:138: if (!PaymentsValidators::isValidCurrencyCodeFormat(item.amount().currency(), &errorMessage)) { To really make this match the spec you probably should also get rid of this verification, because of "user agents MUST not attempt to validate this string" in the spec. Although despite what the spec says we should probably do some kind of sanity check before sending arbitrarily long strings over IPC though...
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981903002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981903002/20001
On 2016/05/16 18:54:50, zino wrote: > On 2016/05/16 16:06:37, Rouslan wrote: > > Thank you for the patch! > > > > It used to be "currencyCode" initially, but I have not had the chance to > update > > the code, while the spec has been evolving. Therefore, please change the > > description to "Update CurrencyAount.idl according to the new spec" or > something > > in that flavor. Also please note that you missed a couple of files in: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a... > > Thank you for review! > > I've just uploaded a new patch set and changed the description. > Could you review it again? > > Thank you. You seem to have missed https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/a... . Please update "currencyCode" to be "currency" here as well. Some bots are failing on the following tests: PaymentRequestTest.UseTheSingleShippingOptionFromPaymentDetailsUpdate PaymentRequestTest.ClearShippingOptionOnPaymentDetailsUpdateWithoutShippingOption Please make sure that your patch does not break these tests. You can run these tests locally via this command: ninja -Cout/Debug webkit_unit_tests && out/Debug/webkit_unit_tests --gtest_filter="PaymentRequestTest.*"
https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:138: if (!PaymentsValidators::isValidCurrencyCodeFormat(item.amount().currency(), &errorMessage)) { On 2016/05/16 19:02:48, Marijn Kruisselbrink wrote: > To really make this match the spec you probably should also get rid of this > verification, because of "user agents MUST not attempt to validate this string" > in the spec. > > Although despite what the spec says we should probably do some kind of sanity > check before sending arbitrarily long strings over IPC though... We would also need to update the validation logic in the browser process: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... Perhaps this can be done in a separate patch? Then this patch can be a simple rename.
Also note that payments/promises-keep-request-alive.html layout test is failing with this patch. Please fix that. You can run it locally like this: ninja -Cout/Debug blink_tests && third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Debug payments
On 2016/05/16 21:39:50, Rouslan wrote: > Also note that payments/promises-keep-request-alive.html layout test is failing > with this patch. Please fix that. You can run it locally like this: > > ninja -Cout/Debug blink_tests && > third_party/WebKit/Tools/Scripts/run-webkit-tests --target=Debug payments That was happening in patch 1. Patch 2 seems to have fixed it.
https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:138: if (!PaymentsValidators::isValidCurrencyCodeFormat(item.amount().currency(), &errorMessage)) { On 2016/05/16 at 21:36:36, Rouslan wrote: > On 2016/05/16 19:02:48, Marijn Kruisselbrink wrote: > > To really make this match the spec you probably should also get rid of this > > verification, because of "user agents MUST not attempt to validate this string" > > in the spec. > > > > Although despite what the spec says we should probably do some kind of sanity > > check before sending arbitrarily long strings over IPC though... > > We would also need to update the validation logic in the browser process: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > Perhaps this can be done in a separate patch? Then this patch can be a simple rename. Sure, that works. And to fully match the spec change this CL links to the amount format regex will need to be updated as well.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2016/05/16 21:36:36, Rouslan wrote: > https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): > > https://codereview.chromium.org/1981903002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:138: if > (!PaymentsValidators::isValidCurrencyCodeFormat(item.amount().currency(), > &errorMessage)) { > On 2016/05/16 19:02:48, Marijn Kruisselbrink wrote: > > To really make this match the spec you probably should also get rid of this > > verification, because of "user agents MUST not attempt to validate this > string" > > in the spec. > > > > Although despite what the spec says we should probably do some kind of sanity > > check before sending arbitrarily long strings over IPC though... > > We would also need to update the validation logic in the browser process: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav... > > Perhaps this can be done in a separate patch? Then this patch can be a simple > rename. Thank you for review. So, in a separate patch, I will have to remove the validation logic in blink side and browser side(chrome/java). Also, need a sanity check before sending arbitrarily long strings over IPC though. Is my understanding correct? As I know, chromium IPC(or mojo?) already has string length check itself. (But I'm not sure.. my memory is fuzzy..) Nevertheless, do we have to add the check logic? Please check my questions. Thank you.
On 2016/05/16 22:58:52, zino wrote: > So, in a separate patch, > I will have to remove the validation logic in blink side and browser > side(chrome/java). > Also, need a sanity check before sending arbitrarily long strings over IPC > though. > Is my understanding correct? You will need to change the existing validation logic to allow arbitrary length strings. Let's limit at something sensible, like 2048 bytes. > As I know, chromium IPC(or mojo? We are using mojo.
On 2016/05/17 00:27:12, Rouslan wrote: > You will need to change the existing validation logic to allow arbitrary length > strings. Let's limit at something sensible, like 2048 bytes. To clarify, this is not necessary for this patch. You should do this in a follow up patch. For this patch, a simple renaming will do. Make sure that all bots are passing first, of course.
Description was changed from ========== PaymentRequest: Update CurrencyAmount.idl according to the latest spec. According to the spec change[1][2], 'currencyCode' property should be 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none ========== to ========== PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl. According to the references[1][2], 'currencyCode' property is renamed to 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none ==========
Description was changed from
==========
PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl.
According to the references[1][2], 'currencyCode' property is renamed to
'currency' to allow flexible currency identifiers.
[1]
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc...
[2]
https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b...
BUG=none
==========
to
==========
PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl.
According to the references[1][2], 'currencyCode' property is renamed to
'currency' to allow flexible currency identifiers.
[1]
https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc...
[2]
https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b...
BUG=none
==========
On 2016/05/18 14:50:01, Rouslan wrote: > On 2016/05/17 00:27:12, Rouslan wrote: > > You will need to change the existing validation logic to allow arbitrary > length > > strings. Let's limit at something sensible, like 2048 bytes. > > To clarify, this is not necessary for this patch. You should do this in a follow > up patch. > > For this patch, a simple renaming will do. Make sure that all bots are passing > first, of course. Okay, I've just renamed it first. PTAL again. I'll fix remaining issue in a follow up CL. Thank you.
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981903002/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mek@: you cool?
On 2016/05/19 at 18:16:10, rouslan wrote: > mek@: you cool? yep, lgtm
The CQ bit was checked by jinho.bang@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1981903002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1981903002/40001
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl. According to the references[1][2], 'currencyCode' property is renamed to 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none ========== to ========== PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl. According to the references[1][2], 'currencyCode' property is renamed to 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl. According to the references[1][2], 'currencyCode' property is renamed to 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none ========== to ========== PaymentRequest: Rename 'currencyCode' to 'currency' in CurrencyAmount.idl. According to the references[1][2], 'currencyCode' property is renamed to 'currency' to allow flexible currency identifiers. [1] https://w3c.github.io/browser-payment-api/specs/paymentrequest.html#h-currenc... [2] https://github.com/w3c/browser-payment-api/commit/563ecd594b5757f5175ee6ede3b... BUG=none Committed: https://crrev.com/c728815891eed7f0fa5d6d7d69376ce2c2e79841 Cr-Commit-Position: refs/heads/master@{#394822} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c728815891eed7f0fa5d6d7d69376ce2c2e79841 Cr-Commit-Position: refs/heads/master@{#394822} |
