Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2348103002:
PaymentRequest: Add support for payerName. (in blink side) (Closed)
|
Created:
4 years, 3 months ago by zino Modified:
4 years, 2 months ago CC:
chromium-reviews, qsr+mojo_chromium.org, Aaron Boodman, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, dglazkov+blink, blink-reviews, darin (slow to review), blink-reviews-api_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentRequest: Add support for payerName. (in blink side)
Support collection of payer name without shipping address.
This CL is a initial implementation for requestPayerName in blink side.
So, it's behind a runtime flag to avoid breaking existing behavior.
If once we implement the related UI, then can remove the flag.
Related spec change:
https://github.com/w3c/browser-payment-api/pull/270
BUG=648051
Committed: https://crrev.com/e25ee6c8c02d34d0176db8db484af8fb92cbab0d
Cr-Commit-Position: refs/heads/master@{#420255}
Patch Set 1 #Patch Set 2 : test #
Total comments: 4
Patch Set 3 : PaymentRequest: Add support for payerName. (in blink side) #
Total comments: 3
Messages
Total messages: 45 (20 generated)
Description was changed from ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. Related spec change: - https://github.com/w3c/browser-payment-api/pull/270 BUG=xxxxxx ========== to ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. Related spec change: - https://github.com/w3c/browser-payment-api/pull/270 BUG=xxxxxx ==========
Description was changed from ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. Related spec change: - https://github.com/w3c/browser-payment-api/pull/270 BUG=xxxxxx ========== to ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. Related spec change: - https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ==========
Description was changed from ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. Related spec change: - https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ========== to ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL includes API changes and related tests, but doesn't include UI change. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ==========
jinho.bang@samsung.com changed reviewers: + rouslan@chromium.org
PTAL
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/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.
Please add to the description information about the flag why you're adding it. I assume that's because the feature needs UI first. Do you mind if I remove the flag once Chrome has this UI? https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp (right): https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp:400: response->payer_name = "Torvalds"; We prefer to use made up names like "Jon Doe" in code. https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (right): https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:50: input->payer_name = "Torvalds"; Ditto about real people names.
Description was changed from ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL includes API changes and related tests, but doesn't include UI change. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ========== to ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL is a initial implementation for requestPayerName in blink side. So, it's behind a runtime flag to avoid breaking existing behavior. If once we implement the related UI, then can remove the flag. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ==========
I addressed your comments. Thank you for review! PTAL https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp (right): https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/OnPaymentResponseTest.cpp:400: response->payer_name = "Torvalds"; On 2016/09/19 08:06:31, rouslan wrote: > We prefer to use made up names like "Jon Doe" in code. Done. https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp (right): https://codereview.chromium.org/2348103002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponseTest.cpp:50: input->payer_name = "Torvalds"; On 2016/09/19 08:06:31, rouslan wrote: > Ditto about real people names. Done.
lgtm
jinho.bang@samsung.com changed reviewers: + foolip@chromium.org
Hi, Philip PTAL. Do we need a intent to implement and ship for this patch?
On 2016/09/19 14:23:17, zino wrote: > Do we need a intent to implement and ship for this patch? No need for an intent to implement and ship.
On 2016/09/19 14:30:18, rouslan wrote: > On 2016/09/19 14:23:17, zino wrote: > > Do we need a intent to implement and ship for this patch? > > No need for an intent to implement and ship. Thank you for clarification.
jinho.bang@samsung.com changed reviewers: + haraken@chromium.org
+ haraken@ for Source/platform/* + foolip@ for Source/public/* PTAL.
lgtm % nit, rouslan@ can say whether the extra feature is really needed. https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentRequest.cpp (right): https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentRequest.cpp:616: if ((m_options.requestPayerName() && response->payer_name.isEmpty()) Off-topic: does this API not allow for a way to request optional information? https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:165: PaymentRequestPayerName status=experimental Does this really need a separate feature, do you expect it to be flipped to status=stable later than PaymentRequest itself?
On 2016/09/19 15:34:40, foolip wrote: > rouslan@ can say whether the extra feature is really needed. We need the feature flag until I implement the UI. Samsung uses a different UI, so it does not make sense for them to implement our UI. Without the UI, a merchant that requests the payer name will not receive the payer name, which is an error condition. So we need to make sure that this part of the API is not available until the UI is implemented. This seems like a natural choice for a flag. Am I misunderstanding how flags are used? > Off-topic: does this API not allow for a way to request optional information? That's in discussions in the Web Payments working group now.
On 2016/09/19 15:46:33, rouslan wrote: > On 2016/09/19 15:34:40, foolip wrote: > > rouslan@ can say whether the extra feature is really needed. > > We need the feature flag until I implement the UI. Samsung uses a different UI, > so it does not make sense for them to implement our UI. Without the UI, a > merchant that requests the payer name will not receive the payer name, which is > an error condition. So we need to make sure that this part of the API is not > available until the UI is implemented. This seems like a natural choice for a > flag. Am I misunderstanding how flags are used? It just seems odd that they are both status=experimental, anyone who then tries to use the API before the UI is done will have the problem you describe, right? But I'll get out of the way and let someone from Samsung judge if this is going to be helpful or not, maybe the feature is enabled/disabled from the outside so that they don't always go together.
On 2016/09/19 16:03:01, foolip wrote: > On 2016/09/19 15:46:33, rouslan wrote: > > On 2016/09/19 15:34:40, foolip wrote: > > > rouslan@ can say whether the extra feature is really needed. > > > > We need the feature flag until I implement the UI. Samsung uses a different > UI, > > so it does not make sense for them to implement our UI. Without the UI, a > > merchant that requests the payer name will not receive the payer name, which > is > > an error condition. So we need to make sure that this part of the API is not > > available until the UI is implemented. This seems like a natural choice for a > > flag. Am I misunderstanding how flags are used? > > It just seems odd that they are both status=experimental, anyone who then tries > to use the API before the UI is done will have the problem you describe, right? > > But I'll get out of the way and let someone from Samsung judge if this is going > to be helpful or not, maybe the feature is enabled/disabled from the outside so > that they don't always go together. Chrome enables PaymentRequest feature in https://cs.chromium.org/chromium/src/content/child/runtime_features.cc?rcl=0&... on Android for all users by default. The other platforms have it disabled by default until UI is implemented on the other platforms.
On 2016/09/19 16:18:27, rouslan wrote: > On 2016/09/19 16:03:01, foolip wrote: > > On 2016/09/19 15:46:33, rouslan wrote: > > > On 2016/09/19 15:34:40, foolip wrote: > > > > rouslan@ can say whether the extra feature is really needed. > > > > > > We need the feature flag until I implement the UI. Samsung uses a different > > UI, > > > so it does not make sense for them to implement our UI. Without the UI, a > > > merchant that requests the payer name will not receive the payer name, which > > is > > > an error condition. So we need to make sure that this part of the API is not > > > available until the UI is implemented. This seems like a natural choice for > a > > > flag. Am I misunderstanding how flags are used? > > > > It just seems odd that they are both status=experimental, anyone who then > tries > > to use the API before the UI is done will have the problem you describe, > right? > > > > But I'll get out of the way and let someone from Samsung judge if this is > going > > to be helpful or not, maybe the feature is enabled/disabled from the outside > so > > that they don't always go together. > > Chrome enables PaymentRequest feature in > https://cs.chromium.org/chromium/src/content/child/runtime_features.cc?rcl=0&... > on Android for all users by default. The other platforms have it disabled by > default until UI is implemented on the other platforms. OK, now it makes sense, thanks :)
platform/ 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/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
jinho.bang@samsung.com changed reviewers: + dcheng@chromium.org
+ dcheng@ for public/platform/modules/payments/payment_request.mojom That's set no parent. PTAL.
jinho.bang@samsung.com changed reviewers: + wfh@chromium.org
+ Will Harris for public/platform/modules/payments/payment_request.mojom PTAL
https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:19: , m_payerName(response->payer_name) Can we write a StructTrait to do this mapping rather than marshalling it manually in the ctor like this? See https://www.chromium.org/developers/design-documents/mojo/type-mapping
looks like dcheng is reviewing. in future don't add more reviewers, as then it's hard to know who is actually meant to be reviewing the code! :)
wfh@chromium.org changed reviewers: - wfh@chromium.org
On 2016/09/20 19:39:09, dcheng wrote: > https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/payments/PaymentResponse.cpp (right): > > https://codereview.chromium.org/2348103002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/payments/PaymentResponse.cpp:19: , > m_payerName(response->payer_name) > Can we write a StructTrait to do this mapping rather than marshalling it > manually in the ctor like this? See > https://www.chromium.org/developers/design-documents/mojo/type-mapping dcheng@, Thank you for review. I agree with you. We should move to typemap. But it doens't look simple to me and this CL is adding support for payerName. So, I filed the related bug(https://crbug.com/649015) and I'd like to work on it in follow-up CL. What do you think?
LGTM, thanks
The CQ bit was checked by jinho.bang@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.
Description was changed from ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL is a initial implementation for requestPayerName in blink side. So, it's behind a runtime flag to avoid breaking existing behavior. If once we implement the related UI, then can remove the flag. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ========== to ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL is a initial implementation for requestPayerName in blink side. So, it's behind a runtime flag to avoid breaking existing behavior. If once we implement the related UI, then can remove the flag. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL is a initial implementation for requestPayerName in blink side. So, it's behind a runtime flag to avoid breaking existing behavior. If once we implement the related UI, then can remove the flag. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 ========== to ========== PaymentRequest: Add support for payerName. (in blink side) Support collection of payer name without shipping address. This CL is a initial implementation for requestPayerName in blink side. So, it's behind a runtime flag to avoid breaking existing behavior. If once we implement the related UI, then can remove the flag. Related spec change: https://github.com/w3c/browser-payment-api/pull/270 BUG=648051 Committed: https://crrev.com/e25ee6c8c02d34d0176db8db484af8fb92cbab0d Cr-Commit-Position: refs/heads/master@{#420255} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/e25ee6c8c02d34d0176db8db484af8fb92cbab0d Cr-Commit-Position: refs/heads/master@{#420255}
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |
